incorrect group value for :name* pattern

[wanderview]

If you run:

const { pathToRegexp } = require('path-to-regexp');
pathToRegexp(':name*')

You will get this regexp:

/^([^\/#\?]+?)*[\/#\?]?$/i

This regexp does not produce a proper matched group value, though. From devtools:

var r = /^([^\/#\?]+?)*[\/#\?]?$/
undefined
r.exec('foobar')
(2) ['foobar', 'r', index: 0, input: 'foobar', groups: undefined]

The matched value is "r" instead of "foobar".

I think path-to-regexp should instead make the current grouping in the regexp non-matching and add a matching group around the repeating * modifier. Like this:

/^((?:[^\/#\?]+?)*)[\/#\?]?$/i

This is my plan for fixing this in URLPattern.

[kettanaito]

I'd like to open a pull request to tackle this. Will add the respective tests to catch any regressions.

[kettanaito]

It looks like the suggested fix cannot be generally applied. When this expression is changed:

path-to-regexp/src/index.ts

Line 566 in 77df638

route += `(${token.pattern})${token.modifier}`;

to:

route += `((?:${token.pattern})${token.modifier})`;

It will result in arbitrary characters being caught in the non-matching group:

• Path: /user(s)?/:userId
• RegExp: ^\/user((?:s)?)\/([^\\/#\\?]+?)
• Match:

/^\/user((?:s)?)\/([^\\/#\\?]+?)/.exec('/user/123')
(3) ["/user/1", "", "1", index: 0, input: "/user/123", groups: undefined]

This is due to the (s)? token getting wrapped in the non-matching group, becoming user((?:s)?) instead of user(s)?.

We can apply the suggested fix only in the case token.modifier === '*'. This retains the previous behavior and fixes the issue above.

I think the same can be said about the + modifier: it has the same issue of not matching the entire path parameter and should also be taken into account within this fix.

const { pathToRegexp } = require('path-to-regexp');
const exp = pathToRegexp(':name*')

exp.exec('foobar')
(2) ['foobar', 'r', index: 0, input: 'foobar', groups: undefined]

[kettanaito]

The fix for both :name* and :name+ paths has been provided and respective tests added in #261.

[wanderview]

I think the fix should also be applied for :name?. Basically if there is a modifier present at all. When there is no modifier present then the old logic is correct. Does that agree with what you are seeing?

I added an end anchor $ to your regexp above and I get:

/^\/user((?:s)?)\/([^\\/#\\?]+?)$/.exec('/user/123')
(3) ['/user/123', '', '123', index: 0, input: '/user/123', groups: undefined]
/^\/user((?:s)?)\/([^\\/#\\?]+?)$/.exec('/users/123')
(3) ['/users/123', 's', '123', index: 0, input: '/users/123', groups: undefined]

[kettanaito]

I’ll add “+” scenario later next week as well.

[wanderview]

I think the fix should also be applied for :name?

You can ignore this feedback from me above. You are indeed correct we don't need the extra non-capturing group for the optional modifier.

[kettanaito]

The fix implemented in #261 is ready for review.

[jeffmcaffer]

FWIW, I'd love a fix for this as well. Just stumbled on the problem. At least for me it seems to only happen with no leading '/'. So you can work around the problem by prefixing the pattern and target with a '/'. Bit of a hassle but seems to work.
Otherwise, awesome library! Thanks.