Skip to content
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

API review: RazorComponentResult #47096

Closed
SteveSandersonMS opened this issue Mar 8, 2023 · 10 comments
Closed

API review: RazorComponentResult #47096

SteveSandersonMS opened this issue Mar 8, 2023 · 10 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-blazor Includes: Blazor, Razor Components feature-full-stack-web-ui Full stack web UI with Blazor
Milestone

Comments

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Mar 8, 2023

Background and Motivation

As part of the Blazor United work, allows developers to render a Razor Component from an MVC action or minimal endpoint.

Proposed API

Result:

namespace Microsoft.AspNetCore.Components.Endpoints
{
    /// <summary>
    /// An <see cref="IResult"/> that renders a Razor Component.
    /// </summary>
    public class RazorComponentResult<TComponent> : RazorComponentResult where TComponent: IComponent
    {
        /// <summary>
        /// Constructs an instance of <see cref="RazorComponentResult"/>.
        /// </summary>
        /// <param name="parameters">Parameters for the component.</param>
        public RazorComponentResult() {}
        public RazorComponentResult(object parameters) {}
        public RazorComponentResult(IReadOnlyDictionary<string, object?> parameters) {}
    }

    /// <summary>
    /// An <see cref="IResult"/> that renders a Razor Component.
    /// </summary>
    public class RazorComponentResult : IResult
    {
        /// <summary>
        /// Constructs an instance of <see cref="RazorComponentResult"/>.
        /// </summary>
        /// <param name="componentType">The type of the component to render. This must implement <see cref="IComponent"/>.</param>
        /// <param name="parameters">Parameters for the component.</param>
        public RazorComponentResult(Type componentType) {}
        public RazorComponentResult(Type componentType, object? parameters) {}
        public RazorComponentResult(Type componentType, IReadOnlyDictionary<string, object?>? parameters) {}

        /// <summary>
        /// Gets the component type.
        /// </summary>
        public Type ComponentType { get; }

        /// <summary>
        /// Gets or sets the Content-Type header for the response.
        /// </summary>
        public string? ContentType { get; set; }

        /// <summary>
        /// Gets or sets the HTTP status code.
        /// </summary>
        public int? StatusCode { get; set; }

        /// <summary>
        /// Gets the parameters for the component.
        /// </summary>
        public IReadOnlyDictionary<string, object?> Parameters { get; }

        /// <summary>
        /// Gets or sets a flag to indicate whether streaming rendering should be prevented. If true, the renderer will
        /// wait for the component hierarchy to complete asynchronous tasks such as loading before supplying the HTML response.
        /// If false, streaming rendering will be determined by the components being rendered.
        ///
        /// The default value is false.
        /// </summary>
        public bool PreventStreamingRendering { get; set; }

        /// <summary>
        /// Requests the service of
        /// <see cref="RazorComponentResultExecutor.ExecuteAsync(HttpContext, RazorComponentResult)" />
        /// to process itself in the given <paramref name="httpContext" />.
        /// </summary>
        /// <param name="httpContext">An <see cref="HttpContext" /> associated with the current request.</param >
        /// <returns >A <see cref="T:System.Threading.Tasks.Task" /> which will complete when execution is completed.</returns >
        public Task ExecuteAsync(HttpContext httpContext) {}
    }
}

Result executor, which is registered in DI as singleton by default:

namespace Microsoft.AspNetCore.Components.Endpoints
{
    /// <summary>
    /// Executes a <see cref="RazorComponentResult"/>.
    /// </summary>
    public class RazorComponentResultExecutor
    {
        /// <summary>
        /// The default content-type header value for Razor Components, <c>text/html; charset=utf-8</c>.
        /// </summary>
        public static readonly string DefaultContentType = "text/html; charset=utf-8";

        /// <summary>
        /// Executes a <see cref="RazorComponentResult"/> asynchronously.
        /// </summary>
        public virtual Task ExecuteAsync(HttpContext httpContext, RazorComponentResult result) {}
    }
}

Usage Examples

We may later add helper methods for creating these without having to new them manually. That is tracked by #47094. However it would also be possible to construct them manually:

Minimal:

endpoints.Map("/mything", () => new RazorComponentResult<MyPageComponent>());

endpoints.Map("/products/{categoryId}", (string categoryId) =>
{
    var componentParameters= new { ItemsPerPage = 10,  Category = categoryId };
    return new RazorComponentResult<MyPageComponent>(componentParameters)
    {
        StatusCode = 212,
        RenderMode = RenderMode.WebAssembly,
        ContentType = "text/html",
    };
}));

MVC:

public class HomeController : Controller
{
    public IResult Home() => new RazorComponentResult<MyPageComponent>();

    public IResult Products(string categoryId)
    {
        var componentParameters = new { ItemsPerPage = 10,  Category = categoryId };
        return new RazorComponentResult<MyPageComponent>(componentParameters)
        {
            StatusCode = 212,
            RenderMode = RenderMode.WebAssembly,
            ContentType = "text/html",
        };
    }
}

Alternative Designs

We considered a few different ways to support passing parameters to the component, including:

A. As a ParameterView
B. As an anonymously-typed object (like the existing Html.RenderComponentAsync helper or <component> tag helper does)
C. As an IReadOnlyDictionary<string, object> (like the existing Html.RenderComponentAsync helper or <component> tag helper does)
D. By actually instantiating the component instance yourself and setting its parameter properties directly

Of these, the ones supported in this PR are B and C, and in both cases it will coerce the value you give to an IReadOnlyDictionary<string, object> because (1) we have to do that anyway, as it's a step on the route to coercing it to a ParameterView which the component actually needs, and (2) having it as a dictionary makes it easy for customers' unit tests to read back the contents of Parameters to make assertions about what their MVC actions are doing.

I haven't put in support for passing a ParameterView directly because it's unlikely anybody would find that convenient. But we could easily add that constructor overload if we wanted.

I haven't put in support for option D, because it would violate the normal component lifecycle by bypassing the SetParametersAsync method. Most components would still work fine in practice, but some legitimately-authored components could fail to work if their parameters were not set via SetParametersAsync. If we were determined to do D, it would probably have to be achieved by using reflection to discover the [Parameter] properties, reading back the values you set, resetting them back to defaults, then preparing a ParameterView that contains the values to be assigned via SetParametersAsync. That's not super lovely because it relies on reflection, and it's always possible that custom getters/setters on the parameter properties would throw if you do something unexpected like explicitly write a null value to them.

Risks

Nothing specific. This is the most basic and obvious way to achieve some functionality we know we want.

@SteveSandersonMS SteveSandersonMS added area-blazor Includes: Blazor, Razor Components api-suggestion Early API idea and discussion, it is NOT ready for implementation feature-full-stack-web-ui Full stack web UI with Blazor labels Mar 8, 2023
@SteveSandersonMS SteveSandersonMS added this to the 8.0-preview3 milestone Mar 8, 2023
@SteveSandersonMS SteveSandersonMS self-assigned this Mar 8, 2023
@DamianEdwards
Copy link
Member

The RazorComponentResult should implement IStatusCodeHttpResult and IContentTypeHttpResult to indicate that it can set status code and/or content-type respectively and allow for simple inspection from filters and validation in application tests.

@DamianEdwards
Copy link
Member

Curious how things like Blazor authorization compose with the minimal API world here. Does it all "just work" using the existing Blazor AuthN/Z primitives thanks to DI or does more stuff need to be flowed here?

@davidfowl
Copy link
Member

I'm not sure we want the existing blazor primitives to light up here. Do we want to run authorization multiple times?

@DamianEdwards
Copy link
Member

Well Blazor absolutely still needs its own level of authorization so that individual components can show different content to different users based on AuthZ policy. Anything applied at the endpoint level would be binary (i.e. can the user hit this endpoint or not) but after that AuthZ still needs to be available (similar to custom logic in an MVC action to return different results to different users for the same resource/endpoint).

@SteveSandersonMS
Copy link
Member Author

The existing ServerAuthenticationStateProvider should already take care of mapping the ASP.NET Core authentication state into the relevant form for Blazor components to display or react to it. We haven't yet implemented the default project template but it might end up being the same as how we set it up by default for Blazor Server today.

@SteveSandersonMS
Copy link
Member Author

The RazorComponentResult should implement IStatusCodeHttpResult and IContentTypeHttpResult to indicate that it can set status code and/or content-type respectively and allow for simple inspection from filters and validation in application tests.

Good point - will be sure to include that in the API review follow-up actions.

@SteveSandersonMS SteveSandersonMS changed the title API proposal: Razor Component IResult API review: Razor Component IResult Mar 15, 2023
@mkArtakMSFT mkArtakMSFT modified the milestones: 8.0-preview7, 8.0-rc1 Jul 24, 2023
@SteveSandersonMS SteveSandersonMS changed the title API review: Razor Component IResult API review: RazorComponentResult Aug 15, 2023
@SteveSandersonMS SteveSandersonMS modified the milestones: 8.0-rc1, 8.0-rc2 Aug 15, 2023
@halter73
Copy link
Member

API Review Notes:

The RazorComponentResult should implement IStatusCodeHttpResult and IContentTypeHttpResult to indicate that it can set status code and/or content-type respectively and allow for simple inspection from filters and validation in application tests.

  • IStatusCodeHttpResult seems to fit the existing shape, so we think this makes sense.
  • Do we like the Microsoft.AspNetCore.Components.Endpoints namespace?
    • What about Microsoft.AspNetCore.Http.HttpResults like Ok<TValue> and other TypedResults?
    • Let's use the HttpResults namespace with the other built-in IResults.
  • Should we seal the result type?
    • Maybe. It's non-breaking to unseal. However RazorComponentResult cannot be sealed since RazorComponentResult<TComponent>, so it'd be inconsistent if we keep the generic type.
  • Why not use helper methods like TypedResults?
    • It feels too niche at the moment.
    • We could consider extension methods after Results.Extensions.RazorComponent<TComponent>().
  • Are we okay with object as an alternative to IReadOnlyDictionary<string, object?>? for parameters.
    • Does this have any trimming implications? Do we need to add any additional annotations to the object parameter constructors to indicate that they are not trim safe? @JamesNK @eerhardt
  • Let's stay consistent and make all the dictionary and object parameters ctor parameter non-nullable.
  • Are we okay with settable properties vs init properties or extra constructor parameters.
    • The existing Results and TypedResults use method parameters exclusively, so we don't have prior art there.
    • We think set; is fine for the less-used properties.
  • Do we need RazorComponentResultExecutor?
    • This Executor pattern seems like a common pattern in MVC that so far we haven't done for the new IResult. We can add it later if need be.

API Approved!

namespace Microsoft.AspNetCore.Http.HttpResults
{
    public class RazorComponentResult<TComponent> : RazorComponentResult where TComponent: IComponent
    {
        public RazorComponentResult() {}

        // NOTE: We should make sure you get proper trimmer warnings on this overload
        public RazorComponentResult(object parameters) {}
        public RazorComponentResult(IReadOnlyDictionary<string, object?> parameters) {}
    }

    public class RazorComponentResult : IResult, IStatusCodeHttpResult, IContentTypeHttpResult
    {
        public RazorComponentResult(Type componentType) {}
        public RazorComponentResult(Type componentType, object parameters) {}
        public RazorComponentResult(Type componentType, IReadOnlyDictionary<string, object?> parameters) {}

        public Type ComponentType { get; }
        public IReadOnlyDictionary<string, object?> Parameters { get; }

        public string? ContentType { get; set; }
        public int? StatusCode { get; set; }
        public bool PreventStreamingRendering { get; set; }

        public Task ExecuteAsync(HttpContext httpContext) {}
    }
}

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Aug 15, 2023
@JamesNK
Copy link
Member

JamesNK commented Aug 16, 2023

Are we okay with object as an alternative to IReadOnlyDictionary<string, object?>? for parameters.

  • Does this have any trimming implications? Do we need to add any additional annotations to the object parameter constructors to indicate that they are not trim safe? @JamesNK @eerhardt

That ctor can be marked as unsafe for trimming/AOT, and people can use the dictionary alternative. That pattern works elsewhere (e.g. RouteValueDictionary).

Do we want to continue to encourage that pattern? IDK. I'll leave that decision up to Steve/Razor team. This issue was created a while ago, before we started supporting AOT in .NET 8, so thinking about whether it is ok to introduce new APIs that are unsafe may have changed in the Razor team.

@SteveSandersonMS
Copy link
Member Author

Should all now be implemented in #50181

@tharlab
Copy link

tharlab commented Aug 31, 2023

I use a lot, minimalapi route/endpoint for blazor
how easy it is to use close to the laravel view/controller way,
and blazor is a plus from dotnet, but I don't really like default blazor's routing way...because it is limited
and I see many other developers also made custom router packages for blazor .
and I myself like to use the power of the minimalapi endpoint /router for Blazor rather than the Blazor default router,

the question in my case .net8 prev7, RazorComponentResult
can't render @inherits LayoutComponentBase in my custom TComponent.razor ??? ,
is this a limitation of using RazorComponentResult? can't @inherits App.razor?

@ghost ghost locked as resolved and limited conversation to collaborators Sep 30, 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 feature-full-stack-web-ui Full stack web UI with Blazor
Projects
None yet
Development

No branches or pull requests

9 participants