Skip to content

[Blazor] Startup APIs API review #49754

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
javiercn opened this issue Jul 31, 2023 · 5 comments
Closed

[Blazor] Startup APIs API review #49754

javiercn opened this issue Jul 31, 2023 · 5 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-blazor Includes: Blazor, Razor Components
Milestone

Comments

@javiercn
Copy link
Member

Background and motivation

These set of APIs define how Razor Component Endpoints are configured in DI and how endpoints to server Razor Component Applications are defined.

Proposed APIs

+static Microsoft.Extensions.DependencyInjection.RazorComponentsServiceCollectionExtensions.AddRazorComponents(this Microsoft.Extensions.DependencyInjection.IServiceCollection! services) -> Microsoft.AspNetCore.Components.Endpoints.IRazorComponentsBuilder!
+static Microsoft.Extensions.DependencyInjection.RazorComponentsServiceCollectionExtensions.AddRazorComponents(this Microsoft.Extensions.DependencyInjection.IServiceCollection! services, System.Action<Microsoft.Extensions.DependencyInjection.RazorComponentOptions!>! setupAction) -> Microsoft.AspNetCore.Components.Endpoints.IRazorComponentsBuilder!
+Microsoft.AspNetCore.Components.WebAssembly.Server.TargetPickerUi.DisplayFirefox(Microsoft.AspNetCore.Http.HttpContext! context) -> System.Threading.Tasks.Task!
+Microsoft.Extensions.DependencyInjection.RazorComponentsBuilderExtensions
+static Microsoft.Extensions.DependencyInjection.RazorComponentsBuilderExtensions.AddWebAssemblyComponents(this Microsoft.AspNetCore.Components.Endpoints.IRazorComponentsBuilder! builder, System.Action<Microsoft.AspNetCore.Components.Endpoints.WebAssemblyComponentsEndpointOptions!>? configure = null) -> Microsoft.AspNetCore.Components.Endpoints.IRazorComponentsBuilder!
+Microsoft.Extensions.DependencyInjection.RazorComponentsBuilderExtensions
+static Microsoft.Extensions.DependencyInjection.RazorComponentsBuilderExtensions.AddServerComponents(this Microsoft.AspNetCore.Components.Endpoints.IRazorComponentsBuilder! builder, System.Action<Microsoft.AspNetCore.Components.Server.CircuitOptions!>? configure = null) -> Microsoft.AspNetCore.Components.Endpoints.IRazorComponentsBuilder!
+Microsoft.AspNetCore.Builder.RazorComponentEndpointConventionBuilder
+Microsoft.AspNetCore.Builder.RazorComponentEndpointConventionBuilder.Add(System.Action<Microsoft.AspNetCore.Builder.EndpointBuilder!>! convention) -> void
+Microsoft.AspNetCore.Builder.RazorComponentEndpointConventionBuilder.AddServerRenderMode() -> Microsoft.AspNetCore.Builder.RazorComponentEndpointConventionBuilder!
+Microsoft.AspNetCore.Builder.RazorComponentEndpointConventionBuilder.AddWebAssemblyRenderMode() -> Microsoft.AspNetCore.Builder.RazorComponentEndpointConventionBuilder!
+Microsoft.AspNetCore.Builder.RazorComponentEndpointConventionBuilder.ApplicationBuilder.get -> Microsoft.AspNetCore.Components.Discovery.ComponentApplicationBuilder!
+Microsoft.AspNetCore.Builder.RazorComponentEndpointConventionBuilder.Finally(System.Action<Microsoft.AspNetCore.Builder.EndpointBuilder!>! finallyConvention) -> void
+Microsoft.AspNetCore.Builder.RazorComponentsEndpointRouteBuilderExtensions
+Microsoft.AspNetCore.Components.Endpoints.WebAssemblyComponentsEndpointOptions
+Microsoft.AspNetCore.Components.Endpoints.WebAssemblyComponentsEndpointOptions.PathPrefix.get -> Microsoft.AspNetCore.Http.PathString
+Microsoft.AspNetCore.Components.Endpoints.WebAssemblyComponentsEndpointOptions.PathPrefix.set -> void
+Microsoft.AspNetCore.Components.Endpoints.WebAssemblyComponentsEndpointOptions.WebAssemblyComponentsEndpointOptions() -> void
+Microsoft.Extensions.DependencyInjection.RazorComponentOptions
+Microsoft.Extensions.DependencyInjection.RazorComponentOptions.FormMappingUseCurrentCulture.get -> bool
+Microsoft.Extensions.DependencyInjection.RazorComponentOptions.FormMappingUseCurrentCulture.set -> void
+Microsoft.Extensions.DependencyInjection.RazorComponentOptions.MaxFormMappingCollectionSize.get -> int
+Microsoft.Extensions.DependencyInjection.RazorComponentOptions.MaxFormMappingCollectionSize.set -> void
+Microsoft.Extensions.DependencyInjection.RazorComponentOptions.MaxFormMappingErrorCount.get -> int
+Microsoft.Extensions.DependencyInjection.RazorComponentOptions.MaxFormMappingErrorCount.set -> void
+Microsoft.Extensions.DependencyInjection.RazorComponentOptions.MaxFormMappingKeySize.get -> int
+Microsoft.Extensions.DependencyInjection.RazorComponentOptions.MaxFormMappingKeySize.set -> void
+Microsoft.Extensions.DependencyInjection.RazorComponentOptions.MaxFormMappingRecursionDepth.get -> int
+Microsoft.Extensions.DependencyInjection.RazorComponentOptions.MaxFormMappingRecursionDepth.set -> void
+Microsoft.Extensions.DependencyInjection.RazorComponentOptions.RazorComponentOptions() -> void
+Microsoft.Extensions.DependencyInjection.RazorComponentsServiceCollectionExtensions

Usage examples

Canonical scenarios

Server Side Rendered components

services.AddRazorComponents();


builder.MapRazorComponents<App>();

Server Side rendering + Interactive server components

services.AddRazorComponents()
  .AddServerComponents();


builder.MapRazorComponents<App>()
  .AddServerRenderMode();

Server Side rendering + Interactive webassembly components

services.AddRazorComponents()
  .AddWebAssemblyComponents();


builder.MapRazorComponents<App>()
  .AddWebAssemblyRenderMode();

Server Side rendering + Interactive server and webassembly components

services.AddRazorComponents()
  .AddServerComponents()
  .AddWebAssemblyComponents();

builder.MapRazorComponents<App>()
  .AddServerRenderMode()
  .AddWebAssemblyRenderMode();

Other advanced scenarios

Configuring global options

services.AddRazorComponents(options =>
{
  options.FormMappingUseCurrentCulture = false;
  options.MaxFormMappingCollectionSize = 100;
  options.MaxFormMappingErrorCount = 100;
  options.MaxFormMappingKeySize = 100;
  options.MaxFormMappingRecursionDepth = 100;
});

Configuring server component options

services.AddRazorComponents()
  .AddServerComponents(options =>
  {
    options.MaxBufferedUnacknowledgedRenderBatches = 5;
    ...
    // These are not new APIs
  });
services.AddRazorComponents()
  .AddServerComponents()
  .AddHubOptions(); // Configure options specific to the SignalR Hub (this is the equivalent of services.AddServerSideBlazor().AddHubOptions(opt => ...))

Risks

@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Jul 31, 2023
@javiercn javiercn added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jul 31, 2023
@ghost
Copy link

ghost commented Jul 31, 2023

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@mkArtakMSFT mkArtakMSFT added this to the 8.0-rc1 milestone Jul 31, 2023
@halter73
Copy link
Member

halter73 commented Jul 31, 2023

API Review Notes:

  • What "builder" are we using in builder.MapRazorComponents<App>(); IEndpointRouteBuilder, so WebApplication or app typically.
  • What's the failure mode if AddRazorComponents without MapRazorComponents or vice versa?
    • We give an error for missing services, but not for "extra" services because there might be extra branches that are hard to analyze.
  • Could we make AddServerRenderMode and AddWebAssemblyRenderMode automatic based on whether AddServerComponents and/or AddWebAssemblyComponents were added?
    • This might be hard because of branching
  • AddServerRenderMode and AddWebAssemblyComponents are not extension methods because they need to run internal logic before you would typically add metadata to an endpoint.
  • What about the name setupAction?
    • This copies MVC. We think we should switch to single overload with a default value: Action<RazorComponentOptions> configure = null for consitency with the other options callbacks we're adding.
  • What about `WebAssemblyComponentsEndpointOptions
  • What does PathPrefix do?
    • Is this referencing a filesystem path? Yes. this is a relative path in the wwwroot. This is why we cannot just rely on route groups.
    • Can we come up with a better name? Not easily. Maybe FrameworkFilesPathsPrefix like in @MackinnonBuck's original PR? Or ContentPathPrefix?
    • It is used in routing, so PathString makes sense as a type.
  • Should form options like FormMappingUseCurrentCulture be a global option, or should we make it per-form?
    • FormMappingUseCurrentCulture defaults to false. If you want complete control, you could manually set it to true and configure the right culture.
    • Let's remove FormMappingUseCurrentCulture because it's a footgun.
  • Can we share form options with minimal APIs?
    • That'd be nice, but we need a design.

Here is our progress so far. In the next meeting will discuss a few more minor things like whether IRazorComponentsBuilder should move to a different namespace, whether RazorComponentOptions should move to a different namespace, if we should seal more classes, and alternate names for the two Microsoft.Extensions.DependencyInjection.RazorComponentsBuilderExtensions classes.

// Microsoft.AspNetCore.Components.Endpoints.dll
namespace Microsoft.AspNetCore.Components.Endpoints;
 
public interface IRazorComponentsBuilder
{
    public IServiceCollection Services { get; }
}

public sealed class WebAssemblyComponentsEndpointOptions
{
    public PathString PathPrefix { get; set; }
}

namespace Microsoft.AspNetCore.Builder;

public class RazorComponentEndpointConventionBuilder : IEndpointConventionBuilder
{
    // internal ctor

    public ComponentApplicationBuilder ApplicationBuilder { get; }

    public RazorComponentEndpointConventionBuilder AddWebAssemblyRenderMode();
    public RazorComponentEndpointConventionBuilder AddServerRenderMode();

    public void Add(Action<EndpointBuilder> convention);
    public void Finally(Action<EndpointBuilder> finallyConvention);
}

namespace Microsoft.Extensions.DependencyInjection;

public static class RazorComponentsServiceCollectionExtensions
{
    public static IRazorComponentsBuilder AddRazorComponents(this IServiceCollection services, Action<RazorComponentOptions>? configure = null);
}

public class RazorComponentOptions
{
    public int MaxFormMappingCollectionSize { get; set; }
    public int MaxFormMappingRecursionDepth { get; set; }
    public int MaxFormMappingErrorCount { get; set; }
    public int MaxFormMappingKeySize { get; set; }
}

// Microsoft.AspNetCore.Components.WebAssembly.Server.dll
namespace Microsoft.Extensions.DependencyInjection;

public static class RazorComponentsBuilderExtensions
{
    public static IRazorComponentsBuilder AddWebAssemblyComponents(this IRazorComponentsBuilder builder, Action<WebAssemblyComponentsEndpointOptions>? configure = null)
}

// Microsoft.AspNetCore.Components.Server.dll
namespace Microsoft.Extensions.DependencyInjection;

public static class RazorComponentsBuilderExtensions
{
    public static IRazorComponentsBuilder AddServerComponents(this IRazorComponentsBuilder builder, Action<CircuitOptions>? configure = null);
}

@javiercn
Copy link
Member Author

javiercn commented Aug 1, 2023

// Microsoft.AspNetCore.Components.Endpoints.dll
-namespace Microsoft.AspNetCore.Components.Endpoints;
+namespace Microsoft.Extensions.DependencyInjection;

public interface IRazorComponentsBuilder
{
    public IServiceCollection Services { get; }
}

-namespace Microsoft.AspNetCore.Components.Endpoints;
+namespace Microsoft.AspNetCore.Components.WebAssembly.Server;

+public sealed class WebAssemblyComponentsEndpointsOptions
{
    public PathString PathPrefix { get; set; }
}
// Move to `WebAssembly.Server` if possible

namespace Microsoft.AspNetCore.Builder;

+public sealed class RazorComponentsEndpointConventionBuilder : IEndpointConventionBuilder
{
    // internal ctor

-    public ComponentApplicationBuilder ApplicationBuilder { get; }

    public RazorComponentsEndpointConventionBuilder AddWebAssemblyRenderMode();
    public RazorComponentsEndpointConventionBuilder AddServerRenderMode();

    public void Add(Action<EndpointBuilder> convention);
    public void Finally(Action<EndpointBuilder> finallyConvention);
}

namespace Microsoft.Extensions.DependencyInjection;

public static class RazorComponentsServiceCollectionExtensions
{
-    public static IRazorComponentsBuilder AddRazorComponents(this IServiceCollection services)
+    public static IRazorComponentsBuilder AddRazorComponents(this IServiceCollection services, Action<RazorComponentsOptions>? configure = null);
}

-namespace Microsoft.Extensions.DependencyInjection;
+namespace Microsoft.AspNetCore.Components.Endpoints;

+ public sealed class RazorComponentsOptions
{
    public int MaxFormMappingCollectionSize { get; set; }
    public int MaxFormMappingRecursionDepth { get; set; }
    public int MaxFormMappingErrorCount { get; set; }
    public int MaxFormMappingKeySize { get; set; }
-    public bool FormMappingUseCurrentCulture { get; set; }
}

// Microsoft.AspNetCore.Components.WebAssembly.Server.dll
namespace Microsoft.Extensions.DependencyInjection;

+public static class WebAssemblyRazorComponentsBuilderExtensions
{
+    public static IRazorComponentsBuilder AddWebAssemblyComponents(this IRazorComponentsBuilder builder)
}

// Microsoft.AspNetCore.Components.Server.dll
namespace Microsoft.Extensions.DependencyInjection;

+public static class ServerRazorComponentsBuilderExtensions
{
    public static IRazorComponentsBuilder AddServerComponents(this IRazorComponentsBuilder builder, Action<CircuitOptions>? configure = null);
}

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Aug 1, 2023
@danroth27 danroth27 modified the milestones: 8.0-rc1, 8.0-rc2 Aug 16, 2023
@halter73
Copy link
Member

halter73 commented Sep 7, 2023

Post API Review fixup notes:

  • We do not have the API updates reflected in this issue yet, but @javiercn plans to add them.
  • What do we think about the proposed RazorComponentOptions to RazorComponentServerOptions rename (made offline)?
    • Will it be confused with "Blazor Server" with a SignalR circuit?
    • RazorComponentServiceOptions?
  • Do we want the RazorComponentsEndpointConventionBuilder.AddRenderMode(IComponentRenderMode renderMode) to exist for use by extension method?
    • Could we make AddServerRenderMode and AddAssemblyRenderMode normal methods rather than extension methods that call AddRenderMode?
      • It would prevent us for adding SignalR specific options to AddServerRenderMode because of layering.

To be continued.

@halter73
Copy link
Member

halter73 commented Sep 7, 2023

Post API Review fixup notes (cont.):

  • WebAssemblyRazorComponentsEndpointConventionBuilderExtensions.WebAssemblyRazorComponentsEndpointConventionBuilderExtensions should take Action<WebAssemblyComponentsEndpointOptions rather than WebAssemblyComponentsEndpointOptions directly.
  • Make AddRenderMode a normal non-extension method in a class tucked away in an uncommon namespace.

@ghost ghost locked as resolved and limited conversation to collaborators Oct 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-blazor Includes: Blazor, Razor Components
Projects
None yet
Development

No branches or pull requests

4 participants