-
Notifications
You must be signed in to change notification settings - Fork 265
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
[Alpha] Add new route functions (...WithExtensions) #634
Conversation
src/Giraffe/EndpointRouting.fs
Outdated
@@ -156,29 +157,71 @@ module Routers = | |||
| CONNECT -> "CONNECT" | |||
| _ -> "" | |||
|
|||
[<RequireQualifiedAccess>] | |||
type AspNetExtension = |
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.
I'm not a fan the DU. This means if someone wants to extend this functionality they have to edit giraffe source code. And if they're doing something custom it's not something we'd want to accept.
It would be better to have a list of IEndpointConventionBuilder -> IEndpointConventionBuilder/unit
functions and maybe provide helpers for known ones we'd want to support.
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.
I changed the code to expose the type ConfigureEndpoint = IEndpointConventionBuilder -> IEndpointConventionBuilder
to the client. Is this aligned to what you had in mind?
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.
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.
From afar this looks good, I agree with Jimmy his comment.
c9775ab
to
35d75d6
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.
Again, fine by me but I would try to get a hold of the folks who actually want this. Give them the alpha and take it from there.
RELEASE_NOTES.md
Outdated
@@ -1,6 +1,27 @@ | |||
Release Notes | |||
============= | |||
|
|||
## 8.0.0 - 202x-xx-xx |
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.
-alpha-001 or however it is done here.
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.
Good catch, done!
src/Giraffe/Giraffe.fsproj
Outdated
@@ -2,7 +2,7 @@ | |||
<PropertyGroup> | |||
<!-- General --> | |||
<AssemblyName>Giraffe</AssemblyName> | |||
<AssemblyVersion>7.0.2</AssemblyVersion> | |||
<AssemblyVersion>8.0.0</AssemblyVersion> |
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.
alpha
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.
Done!
src/Giraffe/EndpointRouting.fs
Outdated
@@ -267,6 +285,7 @@ module Routers = | |||
| NestedEndpoint(t, lst, ce) -> NestedEndpoint(t, lst, ce >> f) | |||
| MultiEndpoint(lst) -> MultiEndpoint(List.map (configureEndpoint f) lst) | |||
|
|||
// XXX should we keep this? |
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.
Is it used anywhere?
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.
I don't think so, but since I'm not sure why it was added in the first place, I'll leave it there and remove the comment.
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.
Removed this comment
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.
Looks great. Agree with @nojaf. Time to let people try it and get feedback.
Description
With this PR, I'm adding some new endpoint routing functions based on what was presented at PR #619. The new routing functions are:
routeWithExtensions
routefWithExtensions
subRouteWithExtensions
There is a new sample showing how to use it with ASP.NET's Rate limiting middleware, a new docs section was added to explain this new feature and more unit tests were added.
From the DOCUMENTATION (added with this PR):
How to test
Related issues