Skip to content

Reduce record usage in published aspnetcore projects #45859

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
1 of 2 tasks
JamesNK opened this issue Jan 4, 2023 · 8 comments · Fixed by #48907
Closed
1 of 2 tasks

Reduce record usage in published aspnetcore projects #45859

JamesNK opened this issue Jan 4, 2023 · 8 comments · Fixed by #48907
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates help wanted Up for grabs. We would accept a PR to help resolve this issue

Comments

@JamesNK
Copy link
Member

JamesNK commented Jan 4, 2023

records have a slim syntax and provide many features, but they also generate a lot of code. And much of that code can't be trimmed. A record can bloat app publish size.

See #45604 (comment) for an example.

  • Audit published aspnetcore projects for record usage. records in test projects is fine.
  • See if there is an analyzer that we can enable to warn about record use, and enable.
@JamesNK
Copy link
Member Author

JamesNK commented Jan 4, 2023

@eerhardt Part of publish size effort.

@javiercn javiercn added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Jan 4, 2023
@halter73 halter73 modified the milestones: 8.0-preview1, .NET 8 Planning Jan 5, 2023
@ghost
Copy link

ghost commented Jan 5, 2023

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@brunolins16
Copy link
Member

brunolins16 commented Jan 24, 2023

Those are all places where record is used in our repo, not including tests and analyzers.

MVC.Core

internal sealed record CollectionResult(IEnumerable<TElement?> Model)

Kestrel

internal sealed record AltSvcHeader(string Value, byte[] RawBytes);

public record InternalHeader(string Identifier, string Name, bool IsPseudoHeader = false);

private readonly record struct OnCloseRegistration(Action<object?> Callback, object? State);

Routing

private readonly record struct DfaBuilderWorkerWorkItem(RouteEndpoint Endpoint, int PrecedenceDigit, List<DfaNode> Parents);

Components

internal record struct InternalAccessTokenResult([property: JsonConverter(typeof(JsonStringEnumConverter))] AccessTokenResultStatus Status, AccessToken Token);

private sealed record UnacknowledgedRenderBatch

Grpc

public sealed record BodyDescriptorInfo(

Shared (Probably used in tests)

public record BrowserOptions(BrowserKind BrowserKind, BrowserTypeLaunchOptions BrowserLaunchOptions, BrowserNewContextOptions DefaultContextOptions);

public record LogEntry(string Message, string Level);

@mitchdenny
Copy link
Member

@eerhardt @captainsafia where do you think this sits in terms of priority right now. I think the work is still valid, but perhaps a lower priority for now?

@eerhardt
Copy link
Member

Yeah, it's pretty low on the priority list IMO. The only case that I would think is high priority is to ensure we don't ship any "new" public record types in .NET 8. If there are ones we added in 8, making them normal classes would be important.

@danmoseley
Copy link
Member

would we take community changes for it - should it be 'help wanted' - or not a good candidate?

@eerhardt eerhardt added the help wanted Up for grabs. We would accept a PR to help resolve this issue label May 24, 2023
@captainsafia
Copy link
Member

Agree that this is a good candidate for help wanted!

I've gone ahead and updated @brunolins16's comment to reflect the the fact that records were removed from RDG so the listi s more accurate for contributors.

@captainsafia captainsafia added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels labels Jun 21, 2023
@david-acker
Copy link
Member

david-acker commented Jun 22, 2023

Here are some other record types that I found in the project (excluding those in analyzer, test, and tools code):

Microsoft.AspNetCore.Components

private record class ComponentTypeInfoCacheEntry(
IComponentRenderMode? ComponentTypeRenderMode,
Action<IServiceProvider, IComponent> PerformPropertyInjection);

private record struct FormDataConverterReadParameters(
ParameterExpression ReaderParam,
ParameterExpression TypeParam,
ParameterExpression OptionsParam,
ParameterExpression ResultParam,
ParameterExpression FoundValueParam);

private record struct NamedEvent(ulong EventHandlerId, int ComponentId, string EventNameId);

private readonly record struct ComponentIdAndDepth(int ComponentId, int Depth);

private record struct MethodInfoData(bool IsSingleArgumentIndexer);

private sealed record BrowserTab
(
string Id,
string Type,
string Url,
string Title,
string DevtoolsFrontendUrl,
string WebSocketDebuggerUrl
);

@ghost ghost locked as resolved and limited conversation to collaborators Aug 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates help wanted Up for grabs. We would accept a PR to help resolve this issue
Projects
No open projects
Status: No status
Development

Successfully merging a pull request may close this issue.

9 participants