Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

feat(routeProvider): Add support to catch-all parameters in routes #1560

Closed
wants to merge 1 commit into from

Conversation

lrlopez
Copy link
Contributor

@lrlopez lrlopez commented Nov 12, 2012

This allows routeProvider to accept parameters that matches substrings even when they contain slashes if they are prefixed with an asterisk instead of a colon.

This PR introduces a new syntax for route params, so we would have:

  • :param for a single param
  • *param for catch-all params

For example, routes like edit/color/:color/largecode/*largecode will match with something like this http://appdomain.com/edit/color/brown/largecode/code/with/slashs

It also matches catch-all routes in the middle of the path.

This allows routeProvider to accept parameters that matches
substrings even when they contain slashes if they are prefixed
with an asterisk instead of a colon.
For example, routes like edit/color/:color/largecode/*largecode
will match with something like this
http://appdomain.com/edit/color/brown/largecode/code/with/slashs.
@lrlopez
Copy link
Contributor Author

lrlopez commented Nov 12, 2012

This PR is related to #1147 which was created by @andresmoschini and assigned to @mhevery. I've just added some tests and documentation.

This is a simplified PR based on @pkozlowski-opensource comments in PR #1559.

@pkozlowski-opensource
Copy link
Member

@lrlopez not the commit looks much better :-)

@lrlopez
Copy link
Contributor Author

lrlopez commented Nov 12, 2012

Thanks :) This is my second PR, so I'm still learning. I'm not used to work in such a big project...

@dcu
Copy link

dcu commented Nov 12, 2012

@lrlopez do you think this is related to #1193 ?

@lrlopez
Copy link
Contributor Author

lrlopez commented Nov 12, 2012

No, it isn't. But PR #1524 does, in fact.

@Enome
Copy link

Enome commented Nov 16, 2012

This is really great for my app. No more ugly encoded paths in my urls!

@lrlopez
Copy link
Contributor Author

lrlopez commented Nov 24, 2012

@pkozlowski-opensource, is there anything else I must do in order to have this PR integrated into the official codebase?

@georgiosd
Copy link

I think that the original code has changed in 1.0.4. Any chance to update the patch? (we manually patched 1.0.3 because we needed the catch-all and now cannot update)

@lrlopez
Copy link
Contributor Author

lrlopez commented Jan 25, 2013

@georgiosd... let me have a look...

@lrlopez
Copy link
Contributor Author

lrlopez commented Jan 25, 2013

I've updated the patch, you can pull it from lrlopez@88bba84

It should be mergeable in 1.0.4 and 1.1.2

@georgiosd
Copy link

Thanks! My JS regex skills are way below par

Sent from a device with a small keyboard

On 25 Jan 2013, at 22:49, "Luis Ramón López" notifications@github.com wrote:

I've updated the patch, you can pull it from lrlopez/angular.js@88bba84

It should be mergable in 1.0.4 and 1.1.2


Reply to this email directly or view it on GitHub.

@georgiosd
Copy link

I've found a bug, I think. I don't have time to jsfiddle it but it seems to work like this:

consider two routes:
(1) /search/location/:id
(2) /search/*query

This seems to cause an endless loop when the URL /search/location/foo is visited

Note: (1) was left there by mistake and it's also a bug on our part. However, it should throw an exception or pick one of the two routes instead of the endless loop

@lrlopez
Copy link
Contributor Author

lrlopez commented Feb 12, 2013

Hmmm... strange. Thanks for reporting the problem. I need to reproduce the problem before I dive into it, so it will take some time.

@georgiosd
Copy link

No probs, let me know if u cant reproduce it with the example above and i ll see if i can create a test case

Sent from a device with a small keyboard

On 12 Feb 2013, at 23:58, "Luis Ramón López" notifications@github.com wrote:

Hmmm... strange. Thanks for reporting the problem. I need to reproduce the problem before I dive into it, so it will take some time.


Reply to this email directly or view it on GitHub.

@mhevery
Copy link
Contributor

mhevery commented Feb 14, 2013

MERGED

@mhevery mhevery closed this Feb 14, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants