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

Path wildcards #37

Closed
recidive opened this Issue Aug 26, 2014 · 13 comments

Comments

Projects
None yet
3 participants
@recidive

I was using pathRegexp() from from Express utils, but since this was removed in 4, I had to migrate to path-to-regexp. I was using it to match an URL only (to get the context for a page) not actually using parameters.

The problem is that /some-path/* URLs stopped working and I need to change it across all the application to use /some-path/(.*).

Since this is an application to be used also by non programmers, /some-path/* would be more straightforward.

Do you have any plans to bring back /some-path/* at some point?

@blakeembrey

This comment has been minimized.

Show comment
Hide comment
@blakeembrey

blakeembrey Aug 26, 2014

Member

@recidive Not currently. Personally it's a bit of an anti-pattern, but if you disagree definitely let me know :)

The reason is, in Express, if you want the wildcard ability you likely should be using .use anyway for mounting (non-trailing route matches). For the couple of cases where you actually want wildcard support, using the manual matching group is easy and verbose enough to show intent.

The downside for that syntax is ambiguity. I cleaned up everything to where there was very little parts ambiguous. Before, everything was interpreted as literal regexp with a couple of cases that were transformed - one being * -> (.*). This made using the asterisk inside a matching group impossible. E.g. /:foo(\\d*). Although it would be possible to give it two separate meanings inside and out of the matching group, I opted to keep things simple.

Member

blakeembrey commented Aug 26, 2014

@recidive Not currently. Personally it's a bit of an anti-pattern, but if you disagree definitely let me know :)

The reason is, in Express, if you want the wildcard ability you likely should be using .use anyway for mounting (non-trailing route matches). For the couple of cases where you actually want wildcard support, using the manual matching group is easy and verbose enough to show intent.

The downside for that syntax is ambiguity. I cleaned up everything to where there was very little parts ambiguous. Before, everything was interpreted as literal regexp with a couple of cases that were transformed - one being * -> (.*). This made using the asterisk inside a matching group impossible. E.g. /:foo(\\d*). Although it would be possible to give it two separate meanings inside and out of the matching group, I opted to keep things simple.

@blakeembrey

This comment has been minimized.

Show comment
Hide comment
@blakeembrey

blakeembrey Aug 26, 2014

Member

Btw, Express 4.x still uses the older syntax so you can always do npm install path-to-regexp@0.1 to have the syntaxes match.

Member

blakeembrey commented Aug 26, 2014

Btw, Express 4.x still uses the older syntax so you can always do npm install path-to-regexp@0.1 to have the syntaxes match.

@recidive

This comment has been minimized.

Show comment
Hide comment
@recidive

recidive Aug 26, 2014

Thanks @blakeembrey. I've changed to 0.1.x and our application is working again.

This is a thing we miss in 1.x and I see this as an upgrade blocking.

Thanks @blakeembrey. I've changed to 0.1.x and our application is working again.

This is a thing we miss in 1.x and I see this as an upgrade blocking.

@blakeembrey

This comment has been minimized.

Show comment
Hide comment
@blakeembrey

blakeembrey Aug 26, 2014

Member

Do you have a use case for it? I don't see an advantage of avoiding three extra characters for the complexity and ambiguity. There are a bunch of things needed for supporting this like /*/:foo and having the asterisk correctly indexed in the params.

Member

blakeembrey commented Aug 26, 2014

Do you have a use case for it? I don't see an advantage of avoiding three extra characters for the complexity and ambiguity. There are a bunch of things needed for supporting this like /*/:foo and having the asterisk correctly indexed in the params.

@recidive

This comment has been minimized.

Show comment
Hide comment
@recidive

recidive Aug 26, 2014

Yes, I understand it's a complex thing.

My use case is that I have a context matching system, so when user access e.g. /user/* I need to run it through all the context conditions that are path based. I.e. say I have 2 contexts with path conditions, one more general with ['/user', '/user/*'] and other more specific with ['/user/edit'] I need both to match.

The issue is that I want to enable non-developer users to configure those contexts and asking them to use (.*) whilst they could just use * would be a bad thing I believe. Our users will not be using regexp 99% of the time.

Yes, I understand it's a complex thing.

My use case is that I have a context matching system, so when user access e.g. /user/* I need to run it through all the context conditions that are path based. I.e. say I have 2 contexts with path conditions, one more general with ['/user', '/user/*'] and other more specific with ['/user/edit'] I need both to match.

The issue is that I want to enable non-developer users to configure those contexts and asking them to use (.*) whilst they could just use * would be a bad thing I believe. Our users will not be using regexp 99% of the time.

@blakeembrey

This comment has been minimized.

Show comment
Hide comment
@blakeembrey

blakeembrey Aug 26, 2014

Member

Perhaps non-developers can utilise /:foo*? Can you quickly explain to me why you aren't using .use('/user') or a route like /user/:user because * matches absolutely everything after the slash? Is there something I can look at to get an understanding of the non-developer user case?

Member

blakeembrey commented Aug 26, 2014

Perhaps non-developers can utilise /:foo*? Can you quickly explain to me why you aren't using .use('/user') or a route like /user/:user because * matches absolutely everything after the slash? Is there something I can look at to get an understanding of the non-developer user case?

@recidive

This comment has been minimized.

Show comment
Hide comment
@recidive

recidive Aug 26, 2014

Our framework does things different from a standard express app. We allow users to create pages (high level routes) via JSON files and want to enable them to do so via UI in the short term.

In a nut shell it work like this:

  1. Users create a page and put a path like /user/:username, or /user or /user/:username/edit or whatever path they want their page to have.
  2. Users create a context with a path condition like ['/user', '/user/*'] and want this context to be triggered when accessing path like the three above. So it can, for example set a layout for the page or add additional blocks of content to the page.

Check this JSON context for the user management extension for an example:

https://github.com/recidive/choko/blob/master/applications/default/extensions/management/contexts/manage.context.json

The Choko framework will then read those JSON files and create the application at runtime based on those pages, contexts and other components without users necessarily having to write JavaScript code.

Our framework does things different from a standard express app. We allow users to create pages (high level routes) via JSON files and want to enable them to do so via UI in the short term.

In a nut shell it work like this:

  1. Users create a page and put a path like /user/:username, or /user or /user/:username/edit or whatever path they want their page to have.
  2. Users create a context with a path condition like ['/user', '/user/*'] and want this context to be triggered when accessing path like the three above. So it can, for example set a layout for the page or add additional blocks of content to the page.

Check this JSON context for the user management extension for an example:

https://github.com/recidive/choko/blob/master/applications/default/extensions/management/contexts/manage.context.json

The Choko framework will then read those JSON files and create the application at runtime based on those pages, contexts and other components without users necessarily having to write JavaScript code.

@blakeembrey

This comment has been minimized.

Show comment
Hide comment
@blakeembrey

blakeembrey Aug 26, 2014

Member

@recidive Some other things you may want to consider too. For /user/:path* or /user/(.+)? the parameter takes into account your prefix so both these rules are the same as your two in one. Should asterisks match this behaviour too? E.g. with /user/* the slash is optional but required if there's a match? I'm still not on board with the change, and to non-developers * and (.*) could mean the same thing, it's just about documentation here - they don't need to learn regexes to make the wording around each the same.

Member

blakeembrey commented Aug 26, 2014

@recidive Some other things you may want to consider too. For /user/:path* or /user/(.+)? the parameter takes into account your prefix so both these rules are the same as your two in one. Should asterisks match this behaviour too? E.g. with /user/* the slash is optional but required if there's a match? I'm still not on board with the change, and to non-developers * and (.*) could mean the same thing, it's just about documentation here - they don't need to learn regexes to make the wording around each the same.

@andineck

This comment has been minimized.

Show comment
Hide comment
@andineck

andineck May 11, 2015

I ran into this problem as well, and it took me a while to find the reason for this unexpected behaviour, since page.js docs are not reflecting the current behaviour.

I just checked express.js deps again:
"path-to-regexp": "0.1.3",

I really think the current version where wildcard does not work anymore as it does in express.js is very confusing for the user. regex is not everyone's favourite and it makes things not very readable.

I can understand, the ambiguity in the implementation, but I would definitively prefer a consistent (as it has been for a while) and simple use of the module, over a simple implementation of the module itself.

I ran into this problem as well, and it took me a while to find the reason for this unexpected behaviour, since page.js docs are not reflecting the current behaviour.

I just checked express.js deps again:
"path-to-regexp": "0.1.3",

I really think the current version where wildcard does not work anymore as it does in express.js is very confusing for the user. regex is not everyone's favourite and it makes things not very readable.

I can understand, the ambiguity in the implementation, but I would definitively prefer a consistent (as it has been for a while) and simple use of the module, over a simple implementation of the module itself.

@blakeembrey

This comment has been minimized.

Show comment
Hide comment
@blakeembrey

blakeembrey May 11, 2015

Member

@andineck I understand the concern, but there shouldn't be a case where you need to use *. Could you give a code snippet where you use it? In 99% of cases, you should be using .use in Express.js which is also a slight optimisation since you don't need to do path matching. For everything else, you can use a regular expression which, I think, is much clearer about the intention.

I understand it doesn't make things much more readable, but I also don't think * is any more readable than /:path* - the latter even gives you a key name. Also, the asterisk behaviour is largely undefined. For instance, it just expanded to (.*) which made key indexes hard to keep in 0.1.x, and it can cause unexpected behaviour. For example, /user/:path*/foo will match /user/123/foo whereas /user/*/foo will match the same except it can match /user//foo since it's just a zero or more matching group. The expansion syntax is a little more complex in that it'll actually take into account what the path looks like.

Edit: I'm not trying to close the conversion, but I want to make sure all the concerns are covered. If there's legitimate reasons for it I think it can be added back. It's just there's no good definition of what it actually does right now.

Edit 2: There are a few other breaking changes in 1.x too, but I think it was necessary to make the module something easily understood and consistent. For instance, there's now a way to reverse paths in the module. This would not have been possible with the old inconsistencies.

Edit 3: Also, /:path* will work with /test/ but not with /test// whereas /* would work with anything (even /test/// or ////). Not say you never want the second case, but I think it's nice to be explicit.

Member

blakeembrey commented May 11, 2015

@andineck I understand the concern, but there shouldn't be a case where you need to use *. Could you give a code snippet where you use it? In 99% of cases, you should be using .use in Express.js which is also a slight optimisation since you don't need to do path matching. For everything else, you can use a regular expression which, I think, is much clearer about the intention.

I understand it doesn't make things much more readable, but I also don't think * is any more readable than /:path* - the latter even gives you a key name. Also, the asterisk behaviour is largely undefined. For instance, it just expanded to (.*) which made key indexes hard to keep in 0.1.x, and it can cause unexpected behaviour. For example, /user/:path*/foo will match /user/123/foo whereas /user/*/foo will match the same except it can match /user//foo since it's just a zero or more matching group. The expansion syntax is a little more complex in that it'll actually take into account what the path looks like.

Edit: I'm not trying to close the conversion, but I want to make sure all the concerns are covered. If there's legitimate reasons for it I think it can be added back. It's just there's no good definition of what it actually does right now.

Edit 2: There are a few other breaking changes in 1.x too, but I think it was necessary to make the module something easily understood and consistent. For instance, there's now a way to reverse paths in the module. This would not have been possible with the old inconsistencies.

Edit 3: Also, /:path* will work with /test/ but not with /test// whereas /* would work with anything (even /test/// or ////). Not say you never want the second case, but I think it's nice to be explicit.

@andineck

This comment has been minimized.

Show comment
Hide comment
@andineck

andineck May 12, 2015

I'm not saying there is a use case that could not be solved with (.*) instead of *.

I am using express.Router on the server side, and page.js in the browser.
Now page.js switched to the new version, express sticks with 0.1.3 (strict).

my path is /upload/* and it is matching e.g. '/upload/img/small'
I am not saying this use case could be rewritten to something like /upload/(.*) or /upload/:path*,
but I also don't think these are more readable than /upload/*

The * was not documented in the best way, but it is documented after all:

Express:

Page.js:

The confusion got even bigger, since page.js uses this to mimic the old * behaviour (but only for straight * and not for things like /api/* with the introduction of this line:
this.path = (path === '*') ? '(.*)' : path;

Personally I think the old version 0.1.3 was simpler to use, and I wish that one could agree on a api, since it is not fun to change this in various places with an updage.
I wonder why express.js is sticking to the version 0.1.3. @dougwilson ?
What's the plan with page.js @shuvalov-anton ?

I'm not saying there is a use case that could not be solved with (.*) instead of *.

I am using express.Router on the server side, and page.js in the browser.
Now page.js switched to the new version, express sticks with 0.1.3 (strict).

my path is /upload/* and it is matching e.g. '/upload/img/small'
I am not saying this use case could be rewritten to something like /upload/(.*) or /upload/:path*,
but I also don't think these are more readable than /upload/*

The * was not documented in the best way, but it is documented after all:

Express:

Page.js:

The confusion got even bigger, since page.js uses this to mimic the old * behaviour (but only for straight * and not for things like /api/* with the introduction of this line:
this.path = (path === '*') ? '(.*)' : path;

Personally I think the old version 0.1.3 was simpler to use, and I wish that one could agree on a api, since it is not fun to change this in various places with an updage.
I wonder why express.js is sticking to the version 0.1.3. @dougwilson ?
What's the plan with page.js @shuvalov-anton ?

@blakeembrey

This comment has been minimized.

Show comment
Hide comment
@blakeembrey

blakeembrey May 12, 2015

Member

You're right. Maybe it can be added back for a little more consistency because I do see it used as a common example everywhere. I expect it to act like the new (.*) though, so it won't be backward compatible with the old behaviour. Also, it won't replace inside of a matching group (E.g. :file(*) won't expand to :file(.*)).

The reason it hasn't been updated in Express.js is because it's a breaking change and needs to wait for 5.x.

Member

blakeembrey commented May 12, 2015

You're right. Maybe it can be added back for a little more consistency because I do see it used as a common example everywhere. I expect it to act like the new (.*) though, so it won't be backward compatible with the old behaviour. Also, it won't replace inside of a matching group (E.g. :file(*) won't expand to :file(.*)).

The reason it hasn't been updated in Express.js is because it's a breaking change and needs to wait for 5.x.

@blakeembrey

This comment has been minimized.

Show comment
Hide comment
@blakeembrey

blakeembrey May 12, 2015

Member

@andineck If you have a chance, please review #54.

Member

blakeembrey commented May 12, 2015

@andineck If you have a chance, please review #54.

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