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

Add helper methods for constructing a RazorComponentResult #47094

Open
SteveSandersonMS opened this issue Mar 8, 2023 · 9 comments
Open

Add helper methods for constructing a RazorComponentResult #47094

SteveSandersonMS opened this issue Mar 8, 2023 · 9 comments
Labels
area-blazor Includes: Blazor, Razor Components feature-full-stack-web-ui Full stack web UI with Blazor Pillar: Complete Blazor Web
Milestone

Comments

@SteveSandersonMS
Copy link
Member

This is to follow up on #47023 (comment) and #47023 (comment)

Currently, you have to construct a RazorComponentResult manually, e.g.:

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

// MVC
public IResult MyPage()
    => new RazorComponentResult<MyPageComponent>(new { Greeting = "Hmm" });

For consistency with other built-in result types, we probably also want helpers so you can do things like:

// Minimal
endpoints.Map("/mypage", () => Results.RazorComponent<MyPageComponent>());

// MVC
public IResult MyPage()
    => RazorComponent<MyPageComponent>(new { Greeting = "Hmm" });
@SteveSandersonMS SteveSandersonMS added area-blazor Includes: Blazor, Razor Components feature-full-stack-web-ui Full stack web UI with Blazor labels Mar 8, 2023
@SteveSandersonMS SteveSandersonMS added this to the .NET 8 Planning milestone Mar 8, 2023
@ghost
Copy link

ghost commented Mar 8, 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.

@captainsafia
Copy link
Member

I'm quoting from the PR and commenting here to avoid clogging up the work you are doing. From #47023:

What about for Minimal APIs? Are minimal people OK with instantiating their own result objects? If not I think I need to put this in the Microsoft.AspNetCore.Http.Results project, which is awkward because then I have to make that reference Mvc.ViewFeatures, or we have to do some bigger refactoring. I don't think we should hang it off Results.Extensions - that would be super weird for a built-in feature. cc @captainsafia

We designed the Results.Extensions API as a way for external libraries to extend the set of available result creators in the library. Actually, one of the motivations at the time was being able to return Liquid template-based views (ref).

Even though the framework is taking advantage of the feature here, it still strikes me as the best place to put the creator method, all things considered. The alternatives mentioned aren't super discoverable and run the risk of masking the feature. What was the weirdness with using the Extensions property to apply this?

@SteveSandersonMS
Copy link
Member Author

SteveSandersonMS commented Mar 20, 2023

What was the weirdness with using the Extensions property to apply this?

The fact that it feels like a place for 2nd-class APIs that aren't really native to the system.

Overall though, is there a reason why we guide people to write code like return Results.Ok(value) instead of return new OkResult(value) or even return new Results.Ok(value) (where Results is a namespace)? Simply using constructors feels more in tune with the C# language and avoid the issue of privileging a specific finite set of result types.

If it were my choice I'd prefer to recommend code like:

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

... rather than:

endpoints.Map("/mypage", () => Results.Extensions.RazorComponent<MyPageComponent>())

However it's not a big deal! If the developers working on Minimal API have a vision of a particular aesthetic then it seems like that's what we should really go for.

@captainsafia
Copy link
Member

captainsafia commented Mar 20, 2023

Overall though, is there a reason why we guide people to write code like return Results.Ok(value) instead of return new OkResult(value) or even return new Results.Ok(value) (where Results is a namespace)?

At the moment, the concrete result types don't have public constructors. There's not an overwhelmingly strong reason to not have public constructs for them. In fact, we've discussed having public constructors in .NET 7 but ultimately stuck with the static creation methods since that was what was historically supported and there were questions about the value of constructors for certain types that didn't have any extension data. See #40802.

cc: @halter73 who can share additional context on the API review that occurred for public Results constructors.

The fact that it feels like a place for 2nd-class APIs that aren't really native to the system.

Yep, this is an understandable reservation. I'd like to think a static method being on the Extensions property doesn't immediately give it a 2nd-class feel given the benefits we get here from using it here but it's a good thing to be mindful of. So far, our options seem to be:

  1. Stick with a public RazorComponentResult constructor which doesn't gel with the static methods that are used elsewhere in minimal APIs
  2. Create a Results.Extensions.RazorComponent method to match the existing experience
  3. Treat the feedback we've gotten here as a motivation to consider making the Results constructor public so they feel like a more first class experience
  4. Do whatever gymnastics are required to get Results.RazorComponent working. I think this is technically complex but putting it on the list for thoroughness.

My preference for this is #2, #1, #3, #4.

Open to other thoughts or proposals that might not be the 4 listed above.

Edit: Option #5 is taking advantage of the upcoming extensions feature in C# to support this scenario.

@halter73
Copy link
Member

Overall though, is there a reason why we guide people to write code like return Results.Ok(value) instead of return new OkResult(value) or even return new Results.Ok(value) (where Results is a namespace)? Simply using constructors feels more in tune with the C# language and avoid the issue of privileging a specific finite set of result types.

I think the main thing is discoverability. Another benefit of methods over constructors is that the generic argument can sometimes be inferred from the argument types. With constructors, you always have to specify the type parameter(s).

@SteveSandersonMS
Copy link
Member Author

OK, thanks for the info @captainsafia and @halter73! The discovery rationale definitely makes sense.

Another benefit of methods over constructors is that the generic argument can sometimes be inferred from the argument types. With constructors, you always have to specify the type parameter(s).

True, but anyone authoring a result type is always free to define their own static factory methods that offer the same type inference.

For now I'm just not going to prioritize creating any special helpers for this and we can see how the situation evolves. Maybe we can use an upcoming C# language feature. Or maybe people will be fine with new. It's a super easy thing to add these any time we like.

@mkArtakMSFT mkArtakMSFT modified the milestones: .NET 8 Planning, Backlog Jun 29, 2023
@ghost
Copy link

ghost commented Jun 29, 2023

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@tharlab
Copy link

tharlab commented Aug 31, 2023

thanks to RazorComponentResult its open blazor to wildest scenario and more easier...
In the past, I saw many other developers made custom endpoint/router packages for blazor.
due to the weakness of blazor route but now it is no longer needed
because aspnet has many alternative powerful router endpoint options to implement on blazor.
and I myself use a lot the power of the minimal web app endpoint/router for Blazor rather than default Blazor router.

@ghost
Copy link

ghost commented Dec 20, 2023

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 6, 2024
@wtgodbe wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
@dotnet dotnet deleted a comment from dotnet-policy-service bot Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components feature-full-stack-web-ui Full stack web UI with Blazor Pillar: Complete Blazor Web
Projects
None yet
Development

No branches or pull requests

9 participants
@halter73 @tharlab @SteveSandersonMS @captainsafia @MackinnonBuck @wtgodbe @mkArtakMSFT and others