feat($route): express style route matching #1745

joshrtay · 2012-12-27T05:59:26Z

visionmedia/express style route matching:

optional params
wildcard params
regexes

.when('/bar/:foo?)
  $location.path('/bar') --> $routePrams: {}
  $location.path('/bar/foovalue') --> $routePrams: {foo: 'foovalue'}

.when('/bar/*')
  $location.path('/bar/foovalue') --> $routeParams: {0: 'foovalue'}

.when('/bar/*.*)
  $location.path('/bar/foo.js') --> $routeParams: {0: 'foo', 1: 'js}

.when(/\/(\d+)/)
  $location.path('/12') --> $routeParams: {0: '12'}

Keyamoon · 2012-12-27T21:10:55Z

If you want your pull requests to be accepted, you need to add tests, change your commit message and sign CLA as described here: http://docs.angularjs.org/misc/contribute

joshrtay · 2012-12-27T21:18:04Z

The commit has tests and I signed the CLA yesterday.

Keyamoon · 2012-12-27T21:19:28Z

You still need to change the commit message. Just letting you know. I had to go through the same process.

joshrtay · 2012-12-27T21:20:13Z

what do you mean by change the commit message?

Keyamoon · 2012-12-27T21:23:44Z

This: https://docs.google.com/document/d/1QrDFcIiPjSLDn3EL15IJygNPiHORgU1_OOAqWjiDU5Y/edit?pli=1#

Taken from this page: http://docs.angularjs.org/misc/contribute

joshrtay · 2012-12-27T21:58:52Z

Thanks for the help. Is there anything else I'm missing?

Keyamoon · 2012-12-27T22:20:27Z

No problem. I think that's it, nothing else missing.

xrado · 2013-02-24T22:45:07Z

this is the feature I'm waiting for, why you are not merge it in?

LucaLanziani · 2013-03-02T11:15:48Z

+1 how long is needed to merge it?

petebacondarwin · 2013-04-30T20:14:32Z

src/ng/route.js

+   * @methodOf ng$routeProvider
+   *
+   * @description
+   * Options fore route matching.


typo "fore" => "for"

petebacondarwin · 2013-04-30T20:22:25Z

petebacondarwin · 2013-04-30T20:32:40Z

@joshrtay - this PR is now a bit outdated and it is difficult to rebase against the current master. Can you take a look at this please?

joshrtay · 2013-05-01T01:02:31Z

ya i'll update it.

joshrtay · 2013-05-01T01:50:32Z

it looks like the way you guys have implemented wildcards is incompatible with the way express does it. wildcards in express are unnamed. how would you like me to proceed?

vojtajina · 2013-05-01T23:08:40Z

@joshrtay thanks for doing this.

Let's add only optional params for now and merge it in.

I think having named wildchars is more expressive so I would prefer keeping that. That means, you can't do stuff like /bar/*.*. I think, in the future, we could make . to be a separator in the same way / is, so you would be able to do stuff like:

.when('user/:id.:format')
// /user/10.json -> {id: '10', format: 'json'}

If we wanna get super crazy, we could later allow regexp matching as well:

.when('/user/:id(\d)'
// /user/23 -> {id: 23}
// /user/x would not match

But at that point, allowing a custom match function rather than a string would be probably better.

Guys @xrado @NSS do you have any other use case that is missing ?

xrado · 2013-05-02T12:07:55Z

optional params are the only thing I miss

joshrtay · 2013-05-03T01:17:23Z

optional params working

jayzeng · 2013-05-03T06:39:33Z

Long waited feature, definitely 👍

petebacondarwin · 2013-05-03T11:26:24Z

test/ng/routeParamsSpec.js

+
+      $location.path('/bar/foovalue');
+      $rootScope.$digest();
+      expect($routeParams).toEqual({foo: 'foovalue'});


~~We should test that even without the optional value the route still matches - currently you are only checking the routeParams~~ I was thinking that we should check the current route to see that it has been matched correctly but I guess that this is implicit.

What about paths that don't just end in an optional parameter?

/bar/:foo?/edit -> will this match /bar/fooValue/edit and /bar/edit ?

We should have tests of these cases for both ? and * style optional parameters.

joshrtay · 2013-05-03T18:28:27Z

I'm not sure what you mean by the routes still match, but I added some more routeParams tests.

Also, my implementation supports this:

.when('user/:id.:format')
// /user/10.json -> {id: '10', format: 'json'}

I think that named wildcards should be considered very carefully, since it seems to go against wildcard convention. Most people would assume that /Book1/*highlight/edit matches
/Book1/Moby/Chapter/highlight/edit.

Unnamed wildcards, though not as expressive, are more consistent with how wildcards typically work and you can still access the values by position.

petebacondarwin · 2013-05-07T11:08:39Z

@joshrtay & @vojtajina - I kind of agree that the named wildcard format is not so intuitive. I wonder if it would be clearer to use a similar convention to the optional parameter:

/Book1/:highlight*/edit

would match Book1/x/y/z/edit and then routeParams.highlight == 'x/y/z'

petebacondarwin · 2013-05-07T11:26:54Z

In the meantime I think we are about there to merge this.

petebacondarwin · 2013-05-07T11:32:14Z

@joshrtay - sorry but IE doesn't like your code: http://ci.angularjs.org/job/angular.js-pete/139/console :-(

joshrtay · 2013-05-07T19:49:35Z

@petebacondarwin - i dont have a great way of testing on IE. any thoughts on what is causing the errors?

petebacondarwin · 2013-07-13T18:09:45Z

My thought was that these are not like unordered params so optionalParam2
could not be used without optionalParam1.

Pete
...from my mobile.
On 13 Jul 2013 18:27, "Igor Minar" notifications@github.com wrote:

@petebacondarwin https://github.com/petebacondarwin but that means that
for route /path/:optionalParam1?/:optionalParam2? and path /path//foo foowould become
optionalParam1. that's wrong.

Let's say that we start supporting creation of urls from route templates
(which we should), if you generate a link from the template above and hash {optionalParam2:
'foo'} the result should match the template above and result in
routeParams -> {optionalParam2: 'foo'}. If we did what you suggested,
routeParams would contain 'foo' key-ed off of optionalParam1. That's
wrong.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1745#issuecomment-20923267
.

joshrtay · 2013-07-16T21:11:21Z

@IgorMinar that proposal sounds good. I think we should allow /path//foo.

IgorMinar · 2013-07-18T17:08:24Z

ok, I'm fine with /path//foo.

can we move forward and making the code change?

joshrtay · 2013-07-18T19:35:56Z

i'll do it this weekend.

joshrtay · 2013-07-24T00:01:59Z

src/ng/route.js

   *
-   *    For example, routes like `/color/:color/largecode/*largecode/edit` will match
+   *    For example, routes like `/color/:color/largecode/:largecode*\/edit` will match


had to add \ so */ didn't end the comment. will this be a problem for the docs?

joshrtay · 2013-07-24T00:03:54Z

code changes were made. let me know what you think.

Added new route matching capabilities: - optional param Changed route matching syntax: - named wildcard

joshrtay · 2013-07-30T21:05:25Z

sorry that last commit wasn't rebased against master

joshrtay · 2013-08-05T02:29:48Z

@IgorMinar waiting for some input on the changes ... would you like me to submit the syntax change to wildcard params as a new pull request?

jbdeboer · 2013-08-09T19:53:28Z

I am seeing a few test failures on this PR.

$route events should NOT load cross domain templates by default
ngView animations should not double compile when the route changes

Investigating further.

Rebasing this CL on-top of master fixes the failures. Yay.

joshrtay · 2013-08-09T20:01:36Z

where are you seeing those test failures?

as far as I can tell, all the tests are passing.

jbdeboer · 2013-08-09T20:10:14Z

All tests are passing for me when I rebase this CL; it could have been my environment.

jbdeboer · 2013-08-09T20:11:03Z

src/ngRoute/route.js

   *
-   *    For example, routes like `/color/:color/largecode/*largecode/edit` will match
+   *    For example, routes like `/color/:color/largecode/:largecode*\/edit` will match


Does the backslash before /edit belong?

i want it to read /color/:color/largecode/:largecode*/edit, but the */ ends the comment. what should i do?

Ha! I missed that.

jbdeboer · 2013-08-10T00:16:38Z

I added a few more test and fixed the style comments in https://github.com/jbdeboer/angular.js/tree/pr1745

Looks good to merge and I will do the merge shortly.

jbdeboer · 2013-08-10T01:49:06Z

Hmm; taking another look at this PR: @IgorMinar's spec said "an optional param cannot be be followed by a string literal, required or wildcard param"

There doesn't seem to be any code that enforced that requirement. The tests check for this case explicitly: '/test/:opt?/:baz/edit

Should we add an explicit check for trailing parameters after an optional?

joshrtay · 2013-08-10T02:45:59Z

Is that something we really want to enforce? It seems ugly, but it is
valid.

On Friday, August 9, 2013, James deBoer wrote:

Hmm; taking another look at this PR: @IgorMinarhttps://github.com/IgorMinar's
spec said "an optional param cannot be be followed by a string literal,
required or wildcard param"

There doesn't seem to be any code that enforced that requirement. The
tests check for this case explicitly: '/test/:opt?/:baz/edit

Should we add an explicit check for trailing parameters after an optional?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1745#issuecomment-22432080
.

jbdeboer · 2013-08-11T01:15:47Z

I would like to hear from @IgorMinar, @petebacondarwin or @vojtajina who were involved in the discussion about literals or required parameters after an optional parameter and have more context.

Re-reading the discussion, the justification for optional parameters seems thin. I see it is a convenience in some situations. However, using optional parameters moves routing decisions out of the route declaration and into controllers (e.g. I can imagine code that checks for an empty optional parameter and then loads a completely different view).

By banning required parameters and string literals after the optional parameters, we would be purposely limited the power, and thus the abuse, of optional parameters.

On the other hand, there is no technical reason for such a ban. Getting this feature into the hands of developers would help us understand how they use it.

jbdeboer · 2013-08-12T18:14:15Z

I talked to Igor and he is good with this implementation that allows literals and required parameters after optional parameters.

Landed as 04cebcc

@joshrtay Thank you so much for work through this multi-month PR!

joshrtay · 2013-08-12T23:38:41Z

@jbdeboer no problem. glad to help.

joshrtay closed this Jan 9, 2013

IgorMinar reopened this Feb 25, 2013

petebacondarwin reviewed Apr 30, 2013
View reviewed changes

src/ng/route.js

* @methodOf ng$routeProvider

*

* @description

* Options fore route matching.

Copy link

Contributor

petebacondarwin Apr 30, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo "fore" => "for"

petebacondarwin reviewed May 3, 2013
View reviewed changes

joshrtay reviewed Jul 24, 2013
View reviewed changes

feat($route): express style route matching

d553b71

Added new route matching capabilities: - optional param Changed route matching syntax: - named wildcard

joshrtay mentioned this pull request Jul 30, 2013

refactor(ngRoute): compile the route once and reuse the regex #3393

Closed

jbdeboer reviewed Aug 9, 2013
View reviewed changes

jbdeboer closed this Aug 12, 2013

alex3683 mentioned this pull request Sep 8, 2014

AngularJS 1.2.x doesn't support routes with empty parameters LaxarJS/laxar#65

Closed

guypod mentioned this pull request Jan 1, 2024

[Snyk] Security upgrade @google-cloud/storage from 1.7.0 to 5.17.0 guypod/angular.js#20

Open

takeratta mentioned this pull request Jan 2, 2024

[Snyk] Security upgrade @google-cloud/storage from 1.7.0 to 5.17.0 takeratta/angular.js#4

Open

guypod mentioned this pull request Jan 6, 2024

[Snyk] Security upgrade @google-cloud/storage from 1.7.0 to 5.17.0 guypod/angular.js#21

Open

takeratta mentioned this pull request Jan 6, 2024

[Snyk] Security upgrade @google-cloud/storage from 1.7.0 to 5.17.0 takeratta/angular.js#5

Open

guypod mentioned this pull request Mar 16, 2024

[Snyk] Security upgrade @google-cloud/storage from 1.7.0 to 5.17.0 guypod/angular.js#22

Open

takeratta mentioned this pull request Mar 18, 2024

[Snyk] Security upgrade @google-cloud/storage from 1.7.0 to 5.17.0 takeratta/angular.js#6

Open

feat($route): express style route matching #1745

feat($route): express style route matching #1745

Conversation

joshrtay commented Dec 27, 2012

Keyamoon commented Dec 27, 2012

joshrtay commented Dec 27, 2012

Keyamoon commented Dec 27, 2012

joshrtay commented Dec 27, 2012

Keyamoon commented Dec 27, 2012

joshrtay commented Dec 27, 2012

Keyamoon commented Dec 27, 2012

xrado commented Feb 24, 2013

LucaLanziani commented Mar 2, 2013

petebacondarwin Apr 30, 2013

Choose a reason for hiding this comment

petebacondarwin commented Apr 30, 2013

petebacondarwin commented Apr 30, 2013

joshrtay commented May 1, 2013

joshrtay commented May 1, 2013

vojtajina commented May 1, 2013

xrado commented May 2, 2013

joshrtay commented May 3, 2013

jayzeng commented May 3, 2013

petebacondarwin May 3, 2013

Choose a reason for hiding this comment

joshrtay commented May 3, 2013

petebacondarwin commented May 7, 2013

petebacondarwin commented May 7, 2013

petebacondarwin commented May 7, 2013

joshrtay commented May 7, 2013

petebacondarwin commented Jul 13, 2013

joshrtay commented Jul 16, 2013

IgorMinar commented Jul 18, 2013

joshrtay commented Jul 18, 2013

joshrtay Jul 24, 2013

Choose a reason for hiding this comment

joshrtay commented Jul 24, 2013

joshrtay commented Jul 30, 2013

joshrtay commented Aug 5, 2013

jbdeboer commented Aug 9, 2013

joshrtay commented Aug 9, 2013

jbdeboer commented Aug 9, 2013

jbdeboer Aug 9, 2013

Choose a reason for hiding this comment

joshrtay Aug 9, 2013

Choose a reason for hiding this comment

jbdeboer Aug 10, 2013

Choose a reason for hiding this comment

jbdeboer commented Aug 10, 2013

jbdeboer commented Aug 10, 2013

joshrtay commented Aug 10, 2013

jbdeboer commented Aug 11, 2013

jbdeboer commented Aug 12, 2013

joshrtay commented Aug 12, 2013