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

Allow external libraries to extend Results #35508

Closed
sebastienros opened this issue Aug 19, 2021 · 8 comments · Fixed by #35571
Closed

Allow external libraries to extend Results #35508

sebastienros opened this issue Aug 19, 2021 · 8 comments · Fixed by #35571
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels feature-minimal-actions Controller-like actions for endpoint routing
Milestone

Comments

@sebastienros
Copy link
Member

sebastienros commented Aug 19, 2021

When using minimal APIs, external libraries could want to expose custom results, like views. There is no way to add custom extension methods to the Results class. However a solution could be to expose a public property e.g. Results.Custom that could be used to expose other types of results.

Today the only solution is for each library to create their own factory class, or return an IResult instance directly which doesn't help for discoverability, and consistency.

Example:

app.MapGet("/", () =>
{
    return LiquidResults.View("index", new Todo(1, "Go back to work!", false));
});

(https://github.com/sebastienros/fluid/pull/292/files#diff-b2a1bda3022b9e2130cc1ececbfc576ec1f91dca2c65baef80c1604c8c36821eR11-R14)

Which could instead become:

app.MapGet("/", () =>
{
    return Results.Custom.LiquidView("index", new Todo(1, "Go back to work!", false));
});
@davidfowl
Copy link
Member

cc @khellang had similar questions about this.

@rafikiassumani-msft rafikiassumani-msft added area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels feature-minimal-actions Controller-like actions for endpoint routing labels Aug 19, 2021
@DamianEdwards
Copy link
Member

We talked about this when we introduced the Results type and the extensibility limitations of the approach, but I feel like this is a worthwhile addition to enable extensions. It's such a trivial addition it really just comes down to whether we're OK committing to this for the long term.

@sebastienros
Copy link
Member Author

Another alternative is to use context or context.Response as the extension method source as long as it's passed to the lambda. Still less discoverable and minimalistic.

@DamianEdwards
Copy link
Member

Yeah having to accept HttpContext or HttpResponse is unfortunate just to get access to extended IResult helpers, and doesn't align with what we have for the inbox stuff either. I prefer the idea of Results.Custom being the extension point.

@pranavkm
Copy link
Contributor

pranavkm commented Aug 20, 2021

Background and Motivation

We would like for library authors to provide custom IResult instances and for a single place to register these extensions for easier discoverability.

Proposed API

namespace Microsoft.AspNetCore.Http.Results
{
+    // Marker interface for extensibility
+    public interface IResultExtensions
+    {
+    }

    public static class Results
    {
+    public static IResultExtensions Extensions { get; } 
    }
}

Usage Examples

// Library code
public static class LiquidResultExtensions
{
   public static IResult LiquidView(this IResultExtensions resultExtensions, string name)
   {
         return new LiquidView(name);
    }
}

// App code
MapGet("/view", () => Results.Extensions.LiquidView("cool-view"));

Alternative Designs

Users can discover result types from libraries through documentation

Risks

N/A

@davidfowl davidfowl added this to the 6.0-rc1 milestone Aug 20, 2021
@captainsafia captainsafia added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Aug 20, 2021
@captainsafia captainsafia self-assigned this Aug 20, 2021
@ghost
Copy link

ghost commented Aug 20, 2021

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.

1 similar comment
@ghost
Copy link

ghost commented Aug 20, 2021

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.

@pranavkm
Copy link
Contributor

API Review

We considered a few options

  • Not shipping this and hoping a language feature that supersedes this.
  • Using a static holder type e.g.
using static ResultsHolder = Microsoft.AspNetCore.Http;

public static class HttpResults
{
    public static IResults Results { get; }
}

This felt iffy since it's only usable within C# that has global static usings.

At this point, we feel there is a need for an API for the community to rally around, so even if it does get deprecated by a newer language feature, we think it's worthwhile.

Naming:

  • Results.Custom
  • Results.Contributions

Since the type name has IResultExtensions already has Extensions in it, it felt like keeping that name is ok.

There was some concern about using Extensions as a property name, since there's a chance that it can conflict with a namespace, or if multiple libraries add the same named extension, but we can provide guidance here.

@pranavkm pranavkm 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 23, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Sep 22, 2021
@amcasey amcasey added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Jun 2, 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-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels feature-minimal-actions Controller-like actions for endpoint routing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants