-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Introduced new route declaration syntax #2441
Conversation
Should we add an overload to remove the
So it becomes
|
@jchannon yeah, doing that now |
@jchannon pushed + updated description |
@@ -56,7 +56,7 @@ public class Route<T> : Route | |||
/// </summary> | |||
/// <param name="description"></param> | |||
/// <param name="action">The action that should take place when the route is invoked.</param> | |||
public Route(RouteDescription description, RouteAction<T> action) | |||
public Route(RouteDescription description, Func<dynamic, CancellationToken, T> action) |
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.
Shouldn't these return a Task<T>
?
I see you made the CancellationToken optional but would there be a case where you'd want it on a non-async route? |
Why/how would you cancel a synchronous operation using a |
@khellang who's that message for? |
You 😄 |
You wouldn't that's why I'm asking why make it available on a sync route |
Oh, I thought you were asking if we could add it. The main problem right now is that all the methods accept non-async delegates (no |
/// <param name="path">The path that the route will respond to</param> | ||
/// <param name="action">Action that will be invoked when the route it hit</param> | ||
/// <param name="condition">A condition to determine if the route can be hit</param> | ||
protected void AddRoute<T>(string name, string method, string path, Func<NancyContext, bool> condition, Func<dynamic, CancellationToken, T> action) |
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.
Seems odd to swap the condition
and action
args around compared to the verb methods.
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.
Ha ha, well spotted! 😄
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.
They weren't really swapped around, but moved out of necessity. The AddRoute
parameter order is left unchanged while the verb methods have been rearranged because condition
and name
are optional.
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.
Makes sense. Guess I'd have swapped them in AddRoute
too then.
@khellang me and @grumpydev spent some time this morning verifying the behavior of it not being |
5d1740c
to
bf3bbe9
Compare
|
ff9e550
to
b490c6b
Compare
/// <returns>A (hot) task of <see cref="Response"/> instance.</returns> | ||
public override Task<object> Invoke(DynamicDictionary parameters, CancellationToken cancellationToken) | ||
{ | ||
return this.Action.Invoke(parameters, cancellationToken).ContinueWith<object>(t => t.Result, TaskContinuationOptions.OnlyOnRanToCompletion); |
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.
Why do you need the ContinueWith
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.
Because this.Action
returns Task<T>
and we need convert it to a Task<object>
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 noticed further up you called an action like action(...)
but here you're using Action.Invoke(...)
I don't believe it makes much different but, consistency? :)
Looking forward to testing this one, I would imagine this will make it simpler to add Swaggeresque metadata overloads in the future or even as an extension. |
Yup On Tuesday, 17 May 2016, Ovan Crone notifications@github.com wrote:
|
|
||
Post["/login"] = x => { | ||
Post("/login", args => { |
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.
OMG @thecodejunkie can you stop with the inconsistent braces!!! :)
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've not changed a single brace
I approve of this syntax 👍 |
Fowler is agreeing with us.. Anyone else suspicious? 😂 |
So we can close this PR now? |
@phillip-haydon if we have properly reviewed the changes around async stuff then yes.. if no, then we need to review it :P For example are we using |
Reviewed a second time. Didn't find anything 😸 |
Held up by @khellang as always. 👀 |
May I request a new release for this? |
@psibernetic we're working on it. in the meantime you can always grab it of our myget feed which gets built on each new commit to |
Yeap thank you, fellas in chat pointed me there, been testing away. |
The wiki still needs updating, BTW |
@canton7 the wiki will not be updates as the 2.0 packages comes out of pre-release, until then all changes are considered as being pending =) |
My bad, apologies for the noise. |
Are there ever going to be doc updates that use the new syntax? |
Prerequisites
Description
In
2.0-alpha
we changed the route syntax and made all the routesasync
which meant they all had to returnTask<T>
. We also temporarily introducedLegacyNancyModule
which was a custom module class which mimicked the old, synchronous, route behavior with the intent to help people migrate to2.0
and then we would remove it in a later release (perhaps even before 2.0 RTM). As it turns out, forcingTask<T>
on all routes wrecked our syntax a bit it made it difficult to have routes that returned primitives such asint
(status code),HttpStatusCode
,string
(response body) or even to callView
orNegotiate
. We did try some tricks, like custom awaiters and so on which made the syntax a bit nicer but it still forced you to declare all routes asasync
and thenawait
Not too shabby, but it still forces you to declare these simple routes, which aren't async by nature, as async which would cause the entire async state machine to be generated even though you did not need them. Clearly not ideal, and most certainly not very Super-Duper-Happy-Path
Proposal
For quite some time now, we have talked internally about a new route declaration syntax. Not a massive change, but one that would mean we would trade in the indexer syntax
Get["/"]
(they have been a signature mark for Nancy since day one) for a method based approachGet("/")
.See #2441 (comment) for samples
Code change mentions
I have introduced a delegateT RouteAction<T>(dynamic parameters, CancellationToken token)
which represents the body of a routeRoute
abstract and introducedRoute<T>
. The base classRoute
does not contain anAction
anymore, the idea is that a Route doesn't per-say need to have an action it just needs to be invocable so you could inherit different kinds ofRoute
if you have any other desires. TheNancyModule
declarations usesRoute<T>
which has theAction
property of typeRouteAction<T>
that in turn will be called whenRoute.Invoke
is invoked.IntroducedRouteResultHelper
which is used to take a route response, which may or may not be a Task, and coereces the result in a format what we can use it. It does some funky casting with the help of reflection and generic method signatures. This class is used byDefaultRouteInvoker
andDiagnosticsHook
+ in a test where we needed to mock theIRouteInvoker
and have it return a proper response.Get()
,Post()
and so on) onNancyModule
have been madevirtual
so you can override and intercept the declarations if you'd like to do something to it before it is storeddynamic
is only used in the route lambdas now, from there on it should beDynamicDictionairy
Worth noting
Since we are inferringT
you cannot return instances of a private modelFor the same reason you need to explicitly define a modelobject
for routes that returns an anonymous modelGet<object>("/", (args, ct) => new { Name = "Nancy" });
LegacyNancyModule
as it serves very little point now and converting to the new syntax is very fast