-
Notifications
You must be signed in to change notification settings - Fork 266
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 global rate limiting sample and much more #619
Conversation
add ASP.NET extensions configuration to the EndpointRouting mechanism, update samples, making a distinction between GlobalRateLimiting and RateLimiting
7123472
to
489e937
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One user on #618 requesting a feature is typically not a strong enough reason to make drastic changes to the public API. I would only consider it if the demand were significantly higher.
As a workaround, you can use:
type Endpoint =
| SimpleEndpoint of HttpVerb * RouteTemplate * HttpHandler * ConfigureEndpoint
| TemplateEndpoint of
HttpVerb *
RouteTemplate *
RouteTemplateMappings *
(obj -> HttpHandler) *
ConfigureEndpoint
| NestedEndpoint of RouteTemplate * Endpoint list * ConfigureEndpoint
| MultiEndpoint of Endpoint list
#if NET8_0
| NewThingWithRateLimiting of blah
#endif
Additionally, consider introducing a similar DSL function wrapped in #if NET8_0
. I understand this may complicate things, but it's a better approach than altering the API on a whim.
Alternatively, you could release another major version, but be cautious not to have too many, as end-users will always have upgrade work on their side.
On a practical note, launching another major version to drop support for net6.0 after the release of net9.0 in November seems reasonable. Ultimately, I believe this change should be communicated more clearly and not be a side effect of a PR.
@@ -232,39 +270,48 @@ module Routers = | |||
let TRACE = applyHttpVerbToEndpoints TRACE | |||
let CONNECT = applyHttpVerbToEndpoints CONNECT | |||
|
|||
let route (path: string) (handler: HttpHandler) : Endpoint = | |||
SimpleEndpoint(HttpVerb.NotSpecified, path, handler, id) | |||
let route (path: string) (aspNetExtensions: AspNetExtension list) (handler: HttpHandler) : Endpoint = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is massive breaking change is it not?
Is means every existing code with route
would break.
I'd go for the following:
let routeWithExtensions (path: string) (aspNetExtensions: AspNetExtension list) (handler: HttpHandler) : Endpoint = SimpleEndpoint(HttpVerb.NotSpecified, path, handler, id, aspNetExtensions)
let route (path: string) (handler: HttpHandler) : Endpoint = routeWithExtensions(path, [], handler)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, it looks a better path to go
@@ -8,7 +8,7 @@ | |||
<NeutralLanguage>en-GB</NeutralLanguage> | |||
|
|||
<!-- Build settings --> | |||
<TargetFrameworks>net6.0;net7.0;net8.0</TargetFrameworks> | |||
<TargetFrameworks>net8.0</TargetFrameworks> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another breaking change here.
@@ -54,16 +54,16 @@ | |||
<IncludeAssets>build</IncludeAssets> | |||
<PrivateAssets>all</PrivateAssets> | |||
</PackageReference> | |||
<PackageReference Include="G-Research.FSharp.Analyzers" Version="0.10.0"> | |||
<PackageReference Include="G-Research.FSharp.Analyzers" Version="0.11.0"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating analyzers is best handled in a separate PR, to be honest. It can make the current PR noisy, and I generally find that smaller PRs focusing on one thing are easier to review.
👍 |
I'm closing this PR in favor of more succinct ones, which I'll open later, tackling specific points of the ideas presented here. |
let route (path: string) (handler: HttpHandler) : Endpoint = | ||
SimpleEndpoint(HttpVerb.NotSpecified, path, handler, id) | ||
let route (path: string) (aspNetExtensions: AspNetExtension list) (handler: HttpHandler) : Endpoint = | ||
SimpleEndpoint(HttpVerb.NotSpecified, path, handler, id, aspNetExtensions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something that has potential 🤔
This function that currently uses the id function has this type:
type ConfigureEndpoint = IEndpointConventionBuilder -> IEndpointConventionBuilder
I don't know much about it, but maybe we can use it instead of this custom wrapper for Asp.Net extensions https://learn.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.builder.iendpointconventionbuilder?view=aspnetcore-9.0
Description
Warning
This is just experimental. To be honest I don't like this configuration that demands an empty list. I'll ask for more opinions before making this huge change to the Giraffe codebase.
With this PR, I'm:
TODO:
How to test
Check the instructions at the sample's README.md.
Related issues
Related to #618.