search

dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.36k stars 9.99k forks source link

BLAZOR API review #4050

Closed danroth27 closed 5 years ago

danroth27 commented 5 years ago

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.

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 :wink:.

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\ApiReviewer to 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.

image

The immediate action item here is to rename M.A.C.Browser and M.A.C.Browser.JS.

ComponentBase and related types

What 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 InjectAttribute and RouteAttribute - we're parking these names in the components namespace, without an attempt to reconcile them with existing or future types.

Built-in components

We 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.cs file:

I'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.*.js files 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

These APIs are relatively recent, and seem pretty polished.

Parameter Infrastructure

I think the design of these types and naming of Parameter needs a review. ParameterCollectionExtensions probably has no reason to be an extension method.

HttpClient extensions

Can we ship this in a Blazor-specific (non-ASP.NET Core Shared Framework assembly)? I have serious concerns about us shipping HttpClient extensibility in the components assembly, and addressing those concerns will break our present our future.

IUriHelper and navigation

I'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 IUrlHelper in MVC, and the naming feels outdated (referential of helpers in Rails).

These types are in Microsoft.AspNetCore.Components.Routing and probably don't belong there unless they are oriented around something more central. This is a namespace with few types.

ElementRef

Should this be renamed to ElementReference? More formal. Should this be in Microsoft.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.

Layouts

EventCallback and Event Handler types

Should UIMouseEventArgs and other DOM-related types life in Microsoft.AspNetCore.Components.Browser?

We should review the EventCallbackFactory extensions 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 RenderTreeBuilder needs 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.RenderTree probably is too specific. This should be in Microsoft.AspNetCore.Components.Rendering.

IDispatcher

The IDispatcher is hiding out here in the .Rendering namespace. We should consider moving it somewhere more top-level unless we fill up .Rendering with other stuff. I'm making this suggestion because IDispatcher doesn'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.

Implementation 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, RenderBatch and everything else in .Rendering (besides IDispatcher). 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

SteveSandersonMS commented 5 years ago

What else?

rynowak commented 5 years ago

Make all attributes sealed: https://github.com/aspnet/AspNetCore/pull/10236#discussion_r284027229

rynowak commented 5 years ago

Consider the vocabulary for the components we ship: https://github.com/aspnet/AspNetCore/pull/10227/files#r284387196

SteveSandersonMS commented 5 years ago

Figure out how to ship helpers like HttpClient.GetJsonAsync<T> that will work in components that run on both server and client (so we need it to be the same method in both places, not just similar-looking extension methods or such)

So it needs to be in CoreFX or Microsoft.AspNetCore.Metadata, if not in a Components package directly.

danroth27 commented 5 years ago

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).

rynowak commented 5 years ago

I've updated the top post with some initial framing and throughts. I did a pass through the Components assembly so far, and plan to go through others and update the notes.

I'm also going to put together a cool spreadsheet to help with this.

SteveSandersonMS commented 5 years ago

@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.

rynowak commented 5 years ago

awesome. I know I've missed stuff because I only looked through a single assembly so far.

SteveSandersonMS commented 5 years ago

Also consider: https://github.com/aspnet/AspNetCore/pull/11376/files#r296651611