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

Introduced new route declaration syntax #2441

Merged
merged 1 commit into from May 19, 2016

Conversation

Projects
None yet
9 participants
@thecodejunkie
Member

thecodejunkie commented May 11, 2016

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified made sure that I am following the Nancy code style guidelines
  • I have provided test coverage for your change (where applicable)

Description

In 2.0-alpha we changed the route syntax and made all the routes async which meant they all had to return Task<T>. We also temporarily introduced LegacyNancyModule which was a custom module class which mimicked the old, synchronous, route behavior with the intent to help people migrate to 2.0 and then we would remove it in a later release (perhaps even before 2.0 RTM). As it turns out, forcing Task<T> on all routes wrecked our syntax a bit it made it difficult to have routes that returned primitives such as int (status code), HttpStatusCode, string (response body) or even to call View or Negotiate. 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 as async and then await

Get["/"] = async (args, ct) => await 200;

Get["/"] = async (args, ct) => await View["index"];

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 approach Get("/").

See #2441 (comment) for samples

Code change mentions

  • I have introduced a delegate T RouteAction<T>(dynamic parameters, CancellationToken token) which represents the body of a route
  • I have made Route abstract and introduced Route<T>. The base class Route does not contain an Action 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 of Route if you have any other desires. The NancyModule declarations uses Route<T> which has the Action property of type RouteAction<T> that in turn will be called when Route.Invoke is invoked.
  • Introduced RouteResultHelper 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 by DefaultRouteInvoker and DiagnosticsHook + in a test where we needed to mock the IRouteInvoker and have it return a proper response.
  • All route declaration method (Get(), Post() and so on) on NancyModule have been made virtual so you can override and intercept the declarations if you'd like to do something to it before it is stored
  • dynamic is only used in the route lambdas now, from there on it should be DynamicDictionairy

Worth noting

  • Since we are inferring T you cannot return instances of a private model
  • For the same reason you need to explicitly define a model object for routes that returns an anonymous model Get<object>("/", (args, ct) => new { Name = "Nancy" });
  • I have removed LegacyNancyModule as it serves very little point now and converting to the new syntax is very fast

@thecodejunkie thecodejunkie added this to the 2.1 milestone May 11, 2016

@thecodejunkie thecodejunkie changed the title from Introduced new route declaration syntax to [WIP] Introduced new route declaration syntax May 11, 2016

@jchannon

This comment has been minimized.

Show comment
Hide comment
@jchannon

jchannon May 11, 2016

Member

Should we add an overload to remove the CancellationToken from sync routes?

Get("/", (args, ct) => {
   return "Hi"
});

So it becomes

Get("/", (args) => {
   return "Hi"
});
Member

jchannon commented May 11, 2016

Should we add an overload to remove the CancellationToken from sync routes?

Get("/", (args, ct) => {
   return "Hi"
});

So it becomes

Get("/", (args) => {
   return "Hi"
});
@thecodejunkie

This comment has been minimized.

Show comment
Hide comment
@thecodejunkie

thecodejunkie May 11, 2016

Member

@jchannon yeah, doing that now

Member

thecodejunkie commented May 11, 2016

@jchannon yeah, doing that now

@thecodejunkie thecodejunkie referenced this pull request May 11, 2016

Closed

Made the Negotiator awaitable #2422

4 of 4 tasks complete
@thecodejunkie

This comment has been minimized.

Show comment
Hide comment
@thecodejunkie

thecodejunkie May 11, 2016

Member

@jchannon pushed + updated description

Member

thecodejunkie commented May 11, 2016

@jchannon pushed + updated description

@jchannon

This comment has been minimized.

Show comment
Hide comment
@jchannon

jchannon May 11, 2016

Member

I see you made the CancellationToken optional but would there be a case where you'd want it on a non-async route?

Member

jchannon commented May 11, 2016

I see you made the CancellationToken optional but would there be a case where you'd want it on a non-async route?

@khellang

This comment has been minimized.

Show comment
Hide comment
@khellang

khellang May 11, 2016

Member

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 CancellationToken?

Member

khellang commented May 11, 2016

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 CancellationToken?

@jchannon

This comment has been minimized.

Show comment
Hide comment
@jchannon

jchannon May 11, 2016

Member

@khellang who's that message for?

Member

jchannon commented May 11, 2016

@khellang who's that message for?

@khellang

This comment has been minimized.

Show comment
Hide comment
@khellang

khellang May 11, 2016

Member

@khellang who's that message for?

You 😄

Member

khellang commented May 11, 2016

@khellang who's that message for?

You 😄

@jchannon

This comment has been minimized.

Show comment
Hide comment
@jchannon

jchannon May 11, 2016

Member

You wouldn't that's why I'm asking why make it available on a sync route

Member

jchannon commented May 11, 2016

You wouldn't that's why I'm asking why make it available on a sync route

@khellang

This comment has been minimized.

Show comment
Hide comment
@khellang

khellang May 11, 2016

Member

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 Task<T>) return.

Member

khellang commented May 11, 2016

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 Task<T>) return.

@thecodejunkie

This comment has been minimized.

Show comment
Hide comment
@thecodejunkie

thecodejunkie May 12, 2016

Member

@khellang me and @grumpydev spent some time this morning verifying the behavior of it not being Task<T> under the premiss that it would be inferred + verified the behavior of the result coercion (found a bug). It all seem to work as expected in terms of asynchronicity https://gist.github.com/grumpydev/048e8ab6fb390dfa53d77d3cc68a4b19

Member

thecodejunkie commented May 12, 2016

@khellang me and @grumpydev spent some time this morning verifying the behavior of it not being Task<T> under the premiss that it would be inferred + verified the behavior of the result coercion (found a bug). It all seem to work as expected in terms of asynchronicity https://gist.github.com/grumpydev/048e8ab6fb390dfa53d77d3cc68a4b19

@thecodejunkie

This comment has been minimized.

Show comment
Hide comment
@thecodejunkie

thecodejunkie May 16, 2016

Member

ℹ️ Update the route declaration overloads to support both sync and async routes, both with or without a cancellation token (ignore the silly sample routes, just needed compiling routes to show what it is now capable of)

public class RoutingModule : NancyModule
{
    public RoutingModule()
    {
        Get("/", args =>
        {
            return 200;
        });

        Get("/", (args, ct) =>
        {
            return Task.FromResult(200);
        });

        Get("/", async args =>
        {
            return await Task.FromResult(200);
        });

        Get("/", async (args, ct) =>
        {
            return await Task.FromResult(200);
        });
    }
}
Member

thecodejunkie commented May 16, 2016

ℹ️ Update the route declaration overloads to support both sync and async routes, both with or without a cancellation token (ignore the silly sample routes, just needed compiling routes to show what it is now capable of)

public class RoutingModule : NancyModule
{
    public RoutingModule()
    {
        Get("/", args =>
        {
            return 200;
        });

        Get("/", (args, ct) =>
        {
            return Task.FromResult(200);
        });

        Get("/", async args =>
        {
            return await Task.FromResult(200);
        });

        Get("/", async (args, ct) =>
        {
            return await Task.FromResult(200);
        });
    }
}

@thecodejunkie thecodejunkie changed the title from [WIP] Introduced new route declaration syntax to Introduced new route declaration syntax May 16, 2016

@thecodejunkie

This comment has been minimized.

Show comment
Hide comment
@thecodejunkie

thecodejunkie May 16, 2016

Member

All CI builds are green but needs a proper reviewing before we pull it in

ping @NancyFx/most-valued-minions @NancyFx/owners

Member

thecodejunkie commented May 16, 2016

All CI builds are green but needs a proper reviewing before we pull it in

ping @NancyFx/most-valued-minions @NancyFx/owners

/// <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);

This comment has been minimized.

@jchannon

jchannon May 16, 2016

Member

Why do you need the ContinueWith

@jchannon

jchannon May 16, 2016

Member

Why do you need the ContinueWith

This comment has been minimized.

@thecodejunkie

thecodejunkie May 16, 2016

Member

Because this.Action returns Task<T> and we need convert it to a Task<object>

@thecodejunkie

thecodejunkie May 16, 2016

Member

Because this.Action returns Task<T> and we need convert it to a Task<object>

This comment has been minimized.

@phillip-haydon

phillip-haydon May 18, 2016

Member

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? :)

@phillip-haydon

phillip-haydon May 18, 2016

Member

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? :)

@jchannon

This comment has been minimized.

Show comment
Hide comment
@jchannon
Member

jchannon commented May 16, 2016

:shipit:

@psibernetic

This comment has been minimized.

Show comment
Hide comment
@psibernetic

psibernetic May 17, 2016

Contributor

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.

Contributor

psibernetic commented May 17, 2016

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.

@jchannon

This comment has been minimized.

Show comment
Hide comment
@jchannon

jchannon May 17, 2016

Member

Yup

On Tuesday, 17 May 2016, Ovan Crone notifications@github.com wrote:

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.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#2441 (comment)

Member

jchannon commented May 17, 2016

Yup

On Tuesday, 17 May 2016, Ovan Crone notifications@github.com wrote:

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.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#2441 (comment)

Post["/login"] = x => {
Post("/login", args => {

This comment has been minimized.

@phillip-haydon

phillip-haydon May 18, 2016

Member

OMG @thecodejunkie can you stop with the inconsistent braces!!! :)

@phillip-haydon

phillip-haydon May 18, 2016

Member

OMG @thecodejunkie can you stop with the inconsistent braces!!! :)

This comment has been minimized.

@thecodejunkie

thecodejunkie May 18, 2016

Member

I've not changed a single brace

@thecodejunkie

thecodejunkie May 18, 2016

Member

I've not changed a single brace

string apiKey = UserDatabase.ValidateUser((string) this.Request.Form.Username,
(string) this.Request.Form.Password);
Post("/", args =>
{

This comment has been minimized.

@phillip-haydon

phillip-haydon May 18, 2016

Member

I know these are demo project's but the file above this has the brace open on the same line, then this opens on the next line!

I'm becoming the stylecop now.

@phillip-haydon

phillip-haydon May 18, 2016

Member

I know these are demo project's but the file above this has the brace open on the same line, then this opens on the next line!

I'm becoming the stylecop now.

{
public RootModule()
{
Get["/"] = _ => this.Response.AsText("This is a REST API. It is in another VS project to " +
Get("/", args => this.Response.AsText("This is a REST API. It is in another VS project to " +
"demonstrate how a common REST API might behave when " +

This comment has been minimized.

@phillip-haydon

phillip-haydon May 18, 2016

Member

Thing's don't line up anymore!

@phillip-haydon

phillip-haydon May 18, 2016

Member

Thing's don't line up anymore!

Get["NamedRoute", "/namedRoute"] = _ => "I am a named route!";
Get(
name: "NamedRoute",

This comment has been minimized.

@phillip-haydon

phillip-haydon May 18, 2016

Member

This
line is not like the other lines.

@phillip-haydon

phillip-haydon May 18, 2016

Member

This
line is not like the other lines.

@@ -4,12 +4,15 @@ public class MainModule : DiagnosticModule
{
public MainModule()
{
Get["/"] = async (_, __) =>
Get("/", _ =>

This comment has been minimized.

@phillip-haydon

phillip-haydon May 18, 2016

Member

I'm going to be anal now and ask, are we going to be consistent and call it args or _ everywhere?

@phillip-haydon

phillip-haydon May 18, 2016

Member

I'm going to be anal now and ask, are we going to be consistent and call it args or _ everywhere?

Get["/sessions/{id}"] = async (ctx, __) =>
Get("/sessions/{id}", ctx =>

This comment has been minimized.

@phillip-haydon

phillip-haydon May 18, 2016

Member

args _ ctx...

@phillip-haydon

phillip-haydon May 18, 2016

Member

args _ ctx...

@@ -35,7 +33,7 @@ public DefaultRouteInvoker(IResponseNegotiator negotiator)
/// <returns>A <see cref="Response"/> instance that represents the result of the invoked route.</returns>
public async Task<Response> Invoke(Route route, CancellationToken cancellationToken, DynamicDictionary parameters, NancyContext context)
{
dynamic result;
object result;

This comment has been minimized.

@phillip-haydon

phillip-haydon May 18, 2016

Member

Why is this now object?

@phillip-haydon

phillip-haydon May 18, 2016

Member

Why is this now object?

This comment has been minimized.

@thecodejunkie

thecodejunkie May 18, 2016

Member

Because outside of the actual route lambdas, we don't need dynamic .. using dynamic in other places has side effects such as the entire method body being treated as dynamic so the compiler cannot infer a return type etc

@thecodejunkie

thecodejunkie May 18, 2016

Member

Because outside of the actual route lambdas, we don't need dynamic .. using dynamic in other places has side effects such as the entire method body being treated as dynamic so the compiler cannot infer a return type etc

{
this.module = module;
}
public FakeNancyModuleConfigurator AddDeleteRoute(string path)
{
this.module.Delete[path] = parameters => {
this.module.Delete(path, args => {

This comment has been minimized.

@phillip-haydon

phillip-haydon May 18, 2016

Member

AHHH BRACE INCONSISTENCY AGAIN! LOL

@phillip-haydon

phillip-haydon May 18, 2016

Member

AHHH BRACE INCONSISTENCY AGAIN! LOL

new Negotiator(context);
negotiator.WithContentType("text/xml");
var negotiator =
new Negotiator(context);

This comment has been minimized.

@phillip-haydon

phillip-haydon May 18, 2016

Member

Why is this on a new line?

@phillip-haydon

phillip-haydon May 18, 2016

Member

Why is this on a new line?

This comment has been minimized.

@thecodejunkie

thecodejunkie May 18, 2016

Member

Nothing but indentation change

@thecodejunkie

thecodejunkie May 18, 2016

Member

Nothing but indentation change

@davidfowl

This comment has been minimized.

Show comment
Hide comment
@davidfowl

davidfowl May 19, 2016

I approve of this syntax 👍

davidfowl commented May 19, 2016

I approve of this syntax 👍

@grumpydev

This comment has been minimized.

Show comment
Hide comment
@grumpydev

grumpydev May 19, 2016

Member

Fowler is agreeing with us.. Anyone else suspicious? 😂

Member

grumpydev commented May 19, 2016

Fowler is agreeing with us.. Anyone else suspicious? 😂

@phillip-haydon

This comment has been minimized.

Show comment
Hide comment
@phillip-haydon

phillip-haydon May 19, 2016

Member

So we can close this PR now?

Member

phillip-haydon commented May 19, 2016

So we can close this PR now?

@thecodejunkie

This comment has been minimized.

Show comment
Hide comment
@thecodejunkie

thecodejunkie May 19, 2016

Member

@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 ConfigureAwait(false); in relevant places in the change etc?

Member

thecodejunkie commented May 19, 2016

@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 ConfigureAwait(false); in relevant places in the change etc?

@horsdal

This comment has been minimized.

Show comment
Hide comment
@horsdal

horsdal May 19, 2016

Member

Reviewed a second time. Didn't find anything 😸

Member

horsdal commented May 19, 2016

Reviewed a second time. Didn't find anything 😸

@phillip-haydon

This comment has been minimized.

Show comment
Hide comment
@phillip-haydon

phillip-haydon May 19, 2016

Member

Held up by @khellang as always. 👀

Member

phillip-haydon commented May 19, 2016

Held up by @khellang as always. 👀

@jchannon jchannon merged commit 4d9c30e into NancyFx:master May 19, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Finished TeamCity Build Nancy :: Nancy PRs : Tests passed: 2366, ignored: 2
Details
@jchannon

This comment has been minimized.

Show comment
Hide comment
@jchannon

jchannon May 19, 2016

Member

Momentous Day!!

tumblr_ln9eat2mn61qa9fb5o1_500

Member

jchannon commented May 19, 2016

Momentous Day!!

tumblr_ln9eat2mn61qa9fb5o1_500

@psibernetic

This comment has been minimized.

Show comment
Hide comment
@psibernetic

psibernetic May 19, 2016

Contributor

May I request a new release for this?

Contributor

psibernetic commented May 19, 2016

May I request a new release for this?

@thecodejunkie

This comment has been minimized.

Show comment
Hide comment
@thecodejunkie

thecodejunkie May 24, 2016

Member

@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 master

Member

thecodejunkie commented May 24, 2016

@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 master

@psibernetic

This comment has been minimized.

Show comment
Hide comment
@psibernetic

psibernetic May 25, 2016

Contributor

Yeap thank you, fellas in chat pointed me there, been testing away.

Contributor

psibernetic commented May 25, 2016

Yeap thank you, fellas in chat pointed me there, been testing away.

@xt0rted xt0rted referenced this pull request May 27, 2016

Merged

Update route syntax in readme #2468

3 of 4 tasks complete
@canton7

This comment has been minimized.

Show comment
Hide comment
@canton7

canton7 Sep 21, 2016

Contributor

The wiki still needs updating, BTW

Contributor

canton7 commented Sep 21, 2016

The wiki still needs updating, BTW

@thecodejunkie

This comment has been minimized.

Show comment
Hide comment
@thecodejunkie

thecodejunkie Sep 21, 2016

Member

@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 =)

Member

thecodejunkie commented Sep 21, 2016

@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 =)

@canton7

This comment has been minimized.

Show comment
Hide comment
@canton7

canton7 Sep 21, 2016

Contributor

My bad, apologies for the noise.

Contributor

canton7 commented Sep 21, 2016

My bad, apologies for the noise.

@thecodejunkie thecodejunkie referenced this pull request Sep 27, 2016

Open

Async all the things #2576

0 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment