BLAZOR API review #4050
Comments
danroth27
added
bug
What else? |
mkArtakMSFT
added
1 - Ready
task
and removed
bug
mkArtakMSFT
removed
area-mvc
|
Make all attributes sealed: #10236 (comment) |
|
Consider the vocabulary for the components we ship: https://github.com/aspnet/AspNetCore/pull/10227/files#r284387196 |
|
Figure out how to ship helpers like So it needs to be in CoreFX or |
|
I think we need to do some work on the shape of the HttpClient JSON helpers as well. As they stand, you don't have any reasonable way of handling responses other than a 200. I think what we want is closer to the Web API Client model, but tied specifically to JSON (no support for additional formatters or content negotiation). |
|
I've updated the top post with some initial framing and throughts. I did a pass through the I'm also going to put together a cool spreadsheet to help with this. |
|
@rynowak This is great! I've also added a section on "Packages/assemblies" since it's likely we'll want to consider some renaming at that level. |
|
awesome. I know I've missed stuff because I only looked through a single assembly so far. |
ghost
locked as resolved and limited conversation to collaborators
Edit: @rynowak hijacking top post for great justice
Framing Blazor API reviews
I'm suggesting that we bucket API review work into areas based on the user-impact and expected usage of APIs, and from there prioritize and time-box some of the decision making. I'm hoping to use this document to paint a high-level picture rather than "do the work" for everything at once.
I don't think it's super reasonable to have us sit in a room and "do a pass" given how many of these APIs we've built up over time - the discussion would quickly get unfocused. I want everyone's input and involvement in making the first release of Blazor awesome - so I'm going to suggest that we do initial reviews and discussions on github or email and then have meetings as-needed.
Here's a quick shout-out for all of the ideas that were explicitly mentioned in the github issue.
AttributeUsage)It's been a long time since some of us started work on Blazor (over a year and a half for Steve). We should look at the details of everything important with fresh eyes, and make sure it's as good as it can be. We've gone up until this point doing the best we can right now, but we're about to cross the threshold where all of these decisions become permanent. It's scary! But at least we're doing it together.
What are we doing?
I think there are probably two beneficiaries of an API review - us, and unsurprisingly our users. While it might seem like we want different things, I think we usually want the same things in this area. If we can craft APIs with good ergonomics, it's easy to talk about with customers, or write documentation for. If we create good APIs it's easy to scale them up as we work on the framework.
Often this means that we need to do hard work so that users have a good experience. If you've worked on MVC for a while I'm sure you can think of a few examples where we wish we did better 😉.
Areas of focus
Areas are in rough order of priority and attempt to call out of some of things that are important to review and decide. Please feel free to suggest your own ideas or disagreements - but keep in mind that we will have to time-box this work (both reviewing and changing).
I'm not attempting to make a list of everything that has to be reviewed.
Here's a rough categorization and catalog of all of the APIs that exist as of a week ago.
https://microsoft.sharepoint.com/:x:/t/DotNetTeam/ERLn4oNI3ZNHvyqRdklMoBUBKTxzD4jOCbmEO0u50p-zqA?e=4vdrl1
Use:
\\fxcore\tools\apps\ApiReviewerto generate a spreadsheet like this.Work Items
Packages/assemblies
There was a proposal circulated via email to try and align and packages around a single name, as well as clean up some unclarities.
The immediate action item here is to rename
M.A.C.BrowserandM.A.C.Browser.JS.M.A.C.Browser->M.A.C.WebM.A.C.Browser.JS->M.A.C.Web.JSComponentBase and related types
ComponentBaseIComponentIHandleEventand other lifecycle APIsRenderHandleParameterAttributeCascadingParameterAttributeIComponentContextInjectAttributeRouteAttributeWhat we're looking for here is the consistency, naming and predictability of the APIs on
ComponentBase.Things I'm personally interested in:
I'm concerned about the naming of things like
InjectAttributeandRouteAttribute- we're parking these names in the components namespace, without an attempt to reconcile them with existing or future types.Built-in components
InputTextand other form typesEditFormAuthorizeDisplayRouterNavLinkWe may choose to break some of these off into their own discussions depending on the size.
On my mind here is coming up with a vocabulary, or a style-guide for component naming and trying to apply it to these and see if it fits.
Startup experience
The startup experience includes everything Blazor adds to the
Startup.csfile:IServiceCollectionIEndpointRouteBuilderI'm leaving aside client-side Blazor for 3.0 which has a lot more startup code, but doesn't need to be locked down.
What we're looking for here is consistency with the rest of the stack and horizontal scalability. Do we expose the right options? Do we expose details of underlying components like SignalR where important?
JS Interface
The
blazor.*.jsfiles define an API for use in bootstrapping Blazor applications, as well as configuring features like reconnection. We need to do a full pass over this since this API has grown organically.Top-level features
Authentication
AuthenticationStateAuthenticationStateProviderAuthenticationStateChangedHandlerThese APIs are relatively recent, and seem pretty polished.
Parameter Infrastructure
ParameterParameterCollectionParameterEnumeratorParameterCollectionExtensionsI think the design of these types and naming of
Parameterneeds a review.ParameterCollectionExtensionsprobably has no reason to be an extension method.HttpClient extensions
HttpClientJsonExtensionsCan we ship this in a Blazor-specific (non-ASP.NET Core Shared Framework assembly)? I have serious concerns about us shipping
HttpClientextensibility in the components assembly, and addressing those concerns will break our present our future.IUriHelper and navigation
IUriHelperUriHelperBaseNavigationExceptionI'm concerned about the design of this API surface, the naming and its future maintainability. This should probably be an abstract class (or broken up into separate concerns). The naming is too similar to
IUrlHelperin MVC, and the naming feels outdated (referential of helpers in Rails).INavigationInterceptionLocationChangedEventArgsNavLinkMatchThese types are in
Microsoft.AspNetCore.Components.Routingand probably don't belong there unless they are oriented around something more central. This is a namespace with few types.ElementRef
ElementRefShould this be renamed to
ElementReference? More formal. Should this be inMicrosoft.AspNetCore.Components.Browser?Forms
Everything in
Microsoft.AspNetCore.Components.Forms- this is a top level API for extensibility and integration with forms, we need to review all of this.EditContextEditContextDataAnnotationsExtensionsEditContextExpressionExtensionsEditContextFieldClassExtensionsFieldChangedEventArgsFieldIdentifierValidationMessageStoreValidationMessageStoreExpressionExtensionsValidationRequestedEventArgsValidationStateChangedEventArgsLayouts
LayoutAttributeLayoutComponentBaseEventCallback and Event Handler types
EventCallbackUIEventArgsUIMouseEventArgsand other browser/DOM specific APIs likeDataTransferEventCallbackFactoryand extension methodsShould
UIMouseEventArgsand other DOM-related types life inMicrosoft.AspNetCore.Components.Browser?We should review the
EventCallbackFactoryextensions and make sure they aren't too gross.Public infrastructure
These are infrastructure-ish things that need to be public, either to preserve layering/extensibility or to because they are useful to users.
RenderTreeBuilder
The
RenderTreeBuilderneeds to be public so it can be targeted by the compiler. It's also useful for really advanced component authors that need to author in C# for any reason.We should do a pass over these APIs for consistency and naming, but I think largely this is pretty good.
We should consider whether we can make
GetFrames()internal.The namespace
Microsoft.AspNetCore.Components.RenderTreeprobably is too specific. This should be inMicrosoft.AspNetCore.Components.Rendering.IDispatcher
The
IDispatcheris hiding out here in the.Renderingnamespace. We should consider moving it somewhere more top-level unless we fill up.Renderingwith other stuff. I'm making this suggestion becauseIDispatcherdoesn't touch any of our other APIs in this namespace.Compiler infrastructure
There's a variety of types that are in the top-level namespace that should almost certainly not be featured so prominently. These are implementation details of what the compiler does, and usually don't make sense for a user to call. We can't really remove anything from these names, so they will likely become more mess over time. We should put all of these types in a dedicated namespace so they are hidden.
BindAttributesBindElementAttributeBindInputElementAttributeBindMethodsEventHandlerAttributeEventHandlersRuntimeHelpersImplementation Details
I'm really concerned about the number and kinds of things that are public. We haven't done a pass yet and trying to hide implementation details yet, but I think it's warranted.
The big risk here is the
Renderer,RenderBatchand everything else in.Rendering(besidesIDispatcher). We could definitely tell people not to use these types, but the reality is that won't stop them. We need to preserve our ability to work on the render tree format. I don't think we have a critical scenario in 3.0 where anyone but us writes a rendering engine.Appendix A: Links to reference sources
Special mention: no ref assembly for .js Browser.JS
Appendix B: Totally ignorable
Microsoft.AspNetCore.Blazor.Buildhas no APIsMicrosoft.AspNetCore.Blazor.DevServerhas no APIsMicrosoft.AspNetCore.Blazor.Templateshas no APIsMicrosoft.VisualStudio.BlazorExtensionhas no APIsThe text was updated successfully, but these errors were encountered: