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

Optional parameters in routes #25

Merged
merged 4 commits into from Sep 16, 2016

Conversation

Projects
None yet
3 participants
@jods4
Contributor

jods4 commented Aug 2, 2016

Related to aurelia/router#83.

This PR introduces two things.

First it adds support for the ASP.NET route parameter format: /user/{id}.

Idea is that the existing basic format :id continues to work and is supported but without fancy options.

The alternative {id} format supports additional options, with a syntax familiar to ASP.NET devs.

This PR already adds support for optional parameters, like {id?}.

It also checks for default values {id=42} and constraints {id:int} and throws a "not supported" error so that those can safely be added in the future.

I also added tests, including 7 new valid routes that uses the new syntax and 2 test cases for the unsupported defaults and constraints.

Optional parameters in routes
Adds support for the ASP.NET route parameter format: /user/{id} with
support for optional parameters {id?}
@EisenbergEffect

This comment has been minimized.

Show comment
Hide comment
@EisenbergEffect

EisenbergEffect Aug 26, 2016

Member

@bryanrsmith I wonder if I could convince you to take a look at this? ;)

Member

EisenbergEffect commented Aug 26, 2016

@bryanrsmith I wonder if I could convince you to take a look at this? ;)

@bryanrsmith

This comment has been minimized.

Show comment
Hide comment
@bryanrsmith

bryanrsmith Aug 26, 2016

Member

Sure thing.

Member

bryanrsmith commented Aug 26, 2016

Sure thing.

@@ -59,6 +101,18 @@ describe('route recognizer', () => {
expect(recognizer.recognize('/notfound')).toBeUndefined();
});
it('should reject default parameter values', () => {

This comment has been minimized.

@bryanrsmith

bryanrsmith Aug 26, 2016

Member

👍 Good call on reserving this syntax.

@bryanrsmith

bryanrsmith Aug 26, 2016

Member

👍 Good call on reserving this syntax.

@bryanrsmith

This comment has been minimized.

Show comment
Hide comment
@bryanrsmith

bryanrsmith Aug 26, 2016

Member

This looks pretty good to me, @jods4! If we support multiple syntaxes I think they need to support the same features. It's confusing that optional segments only work with one syntax. If we can modify this so that foo/:bar? works too, that'd be great.

@EisenbergEffect Are you happy with the decision to introduce an additional route pattern syntax? There are a couple consequences...

  • We probably need to pick one or the other (presumably the original syntax) for official documentation examples, and it may be confusing for devs to see SO answers or code in existing projects that doesn't match the docs.
  • It may be prohibitively difficult to support the complete syntax of both the Rails syntax and the System.Web.Routing syntax. E.g., Rails specifies constraints and defaults in a separate object instead of as part of the pattern string. Rails syntax also doesn't really hide the underlying regex, so you can do more complicated optional parts, like 'foo/(bar/:baz)?'.

Another approach that may be more flexible might be to extract bits like the parser and make it pluggable. That could let alternate syntaxes be community plugins. That probably requires a much larger refactor, though. I'm not sure it's worth it. Thoughts?

Member

bryanrsmith commented Aug 26, 2016

This looks pretty good to me, @jods4! If we support multiple syntaxes I think they need to support the same features. It's confusing that optional segments only work with one syntax. If we can modify this so that foo/:bar? works too, that'd be great.

@EisenbergEffect Are you happy with the decision to introduce an additional route pattern syntax? There are a couple consequences...

  • We probably need to pick one or the other (presumably the original syntax) for official documentation examples, and it may be confusing for devs to see SO answers or code in existing projects that doesn't match the docs.
  • It may be prohibitively difficult to support the complete syntax of both the Rails syntax and the System.Web.Routing syntax. E.g., Rails specifies constraints and defaults in a separate object instead of as part of the pattern string. Rails syntax also doesn't really hide the underlying regex, so you can do more complicated optional parts, like 'foo/(bar/:baz)?'.

Another approach that may be more flexible might be to extract bits like the parser and make it pluggable. That could let alternate syntaxes be community plugins. That probably requires a much larger refactor, though. I'm not sure it's worth it. Thoughts?

@jods4

This comment has been minimized.

Show comment
Hide comment
@jods4

jods4 Aug 26, 2016

Contributor

@bryanrsmith My thoughts were to keep :param for backward compatibility and as a basic parameter syntax, and to add {param} as an alternative, extended syntax with more features.

I don't see much value in bloating the code base and tests with two different syntaxes that support the same set of options. If we pursue this, I think that would rather call for a pluggable route parser, where everyone is free to pick whatever route syntax they want. I am not sure this is worth it, though. All the systems that I know of impose one syntax and you just go with it.

We should also consider that it may be hard to find equivalent syntax for some features. Adding optional parameters would be easy (:param?), but then if one day we support complex, multiple constraints, or maybe we support having parameters inside a segment (/home/user-{id}-{version}) it may become difficult to fit that in the basic syntax.

Another option, of course, is to stick with the existing syntax, support :id? and don't introduce a new syntax at all.

Contributor

jods4 commented Aug 26, 2016

@bryanrsmith My thoughts were to keep :param for backward compatibility and as a basic parameter syntax, and to add {param} as an alternative, extended syntax with more features.

I don't see much value in bloating the code base and tests with two different syntaxes that support the same set of options. If we pursue this, I think that would rather call for a pluggable route parser, where everyone is free to pick whatever route syntax they want. I am not sure this is worth it, though. All the systems that I know of impose one syntax and you just go with it.

We should also consider that it may be hard to find equivalent syntax for some features. Adding optional parameters would be easy (:param?), but then if one day we support complex, multiple constraints, or maybe we support having parameters inside a segment (/home/user-{id}-{version}) it may become difficult to fit that in the basic syntax.

Another option, of course, is to stick with the existing syntax, support :id? and don't introduce a new syntax at all.

@bryanrsmith

This comment has been minimized.

Show comment
Hide comment
@bryanrsmith

bryanrsmith Sep 3, 2016

Member

Agreed, on all counts.

Another option, of course, is to stick with the existing syntax, support :id? and don't introduce a new syntax at all.

This is my favorite option.

Member

bryanrsmith commented Sep 3, 2016

Agreed, on all counts.

Another option, of course, is to stick with the existing syntax, support :id? and don't introduce a new syntax at all.

This is my favorite option.

jods4 added some commits Sep 6, 2016

Removed constrained syntax
Use existing syntax instead, extended with `?` for optional values, e.g.
`:id?`.
Syntax for default parameters is still reserved, e.g. `:id=0`.
Renamed handlers in tests
Doesn't seem to change the test outcome anyway...
@EisenbergEffect

This comment has been minimized.

Show comment
Hide comment
@EisenbergEffect

EisenbergEffect Sep 14, 2016

Member

@jods4 How do you feel about this? Anything else left you want to do or is it ready for final review?

Member

EisenbergEffect commented Sep 14, 2016

@jods4 How do you feel about this? Anything else left you want to do or is it ready for final review?

@jods4

This comment has been minimized.

Show comment
Hide comment
@jods4

jods4 Sep 14, 2016

Contributor

@EisenbergEffect Sorry, the discussion was a bit split between the PR and the issue...
No at the moment I am ok with the PR state.
It is back to the current syntax only /user/:id? and reserves but does not implement the default syntax /user/:id=3. Note that the latter should be easy to implement.

Having optional parameters would help us a lot in what we do, so I'm looking forward to see that PR merged.

Contributor

jods4 commented Sep 14, 2016

@EisenbergEffect Sorry, the discussion was a bit split between the PR and the issue...
No at the moment I am ok with the PR state.
It is back to the current syntax only /user/:id? and reserves but does not implement the default syntax /user/:id=3. Note that the latter should be easy to implement.

Having optional parameters would help us a lot in what we do, so I'm looking forward to see that PR merged.

@EisenbergEffect

This comment has been minimized.

Show comment
Hide comment
@EisenbergEffect

EisenbergEffect Sep 16, 2016

Member

@jods4 I'm preparing to merge this in. Would you be willing to add some new documentation related to this feature? It would go in the router repo. There's a doc folder in there with an article on router configuration.

Member

EisenbergEffect commented Sep 16, 2016

@jods4 I'm preparing to merge this in. Would you be willing to add some new documentation related to this feature? It would go in the router repo. There's a doc folder in there with an article on router configuration.

@EisenbergEffect

A fantastic contribution. This will be a great benefit to the community.

@EisenbergEffect EisenbergEffect merged commit b36653f into aurelia:master Sep 16, 2016

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@jods4

This comment has been minimized.

Show comment
Hide comment
@jods4

jods4 Sep 17, 2016

Contributor

@EisenbergEffect see aurelia/router#403 for doc. Not an English speaker here, so hopefully it's good enough.

I was thinking about the default value and I'm unsure if we really need it.

  1. unlike languages like C#, we can't know what the target type is (a string, a number, a Date?).
  2. with modern JS, it's easy to implement the default value in the ViewModel, like so:
activate({ id = 0 }) {
  // can use id in the method, with default value 0.
}
Contributor

jods4 commented Sep 17, 2016

@EisenbergEffect see aurelia/router#403 for doc. Not an English speaker here, so hopefully it's good enough.

I was thinking about the default value and I'm unsure if we really need it.

  1. unlike languages like C#, we can't know what the target type is (a string, a number, a Date?).
  2. with modern JS, it's easy to implement the default value in the ViewModel, like so:
activate({ id = 0 }) {
  // can use id in the method, with default value 0.
}
@EisenbergEffect

This comment has been minimized.

Show comment
Hide comment
@EisenbergEffect

EisenbergEffect Sep 17, 2016

Member

It's usually better to err on the side of not adding the feature than adding something you don't necessary need but have to end up supporting (and fixing bugs on) long term :)

Thanks for putting together some docs. I'll review and make any grammar fixes if needed.

Member

EisenbergEffect commented Sep 17, 2016

It's usually better to err on the side of not adding the feature than adding something you don't necessary need but have to end up supporting (and fixing bugs on) long term :)

Thanks for putting together some docs. I'll review and make any grammar fixes if needed.

@jods4

This comment has been minimized.

Show comment
Hide comment
@jods4

jods4 Sep 17, 2016

Contributor

@EisenbergEffect PS: thanks for the nice comment. It was tricky to fit the optional part without changing your DFA building code too much. This new feature will be a great benefit for some patterns we have in our code.

Currently our code uses a convention for the route name with parameter and without so that we can redirect from one to the other. With this contribution we can drop the convention altogether! We are going to refactor our code as soon as this is available from bower.

Contributor

jods4 commented Sep 17, 2016

@EisenbergEffect PS: thanks for the nice comment. It was tricky to fit the optional part without changing your DFA building code too much. This new feature will be a great benefit for some patterns we have in our code.

Currently our code uses a convention for the route name with parameter and without so that we can redirect from one to the other. With this contribution we can drop the convention altogether! We are going to refactor our code as soon as this is available from bower.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment