`?` in optional capture group isn't connected to group in output - named param about path-to-regexp HOT 11 CLOSED

Your fix only works accidentally because of #286. You should be escaping the inner parentheses. This isn’t a bug, just confusion on what is happening here - your first closing paren is closing the entire regex.

What version are you using?

from path-to-regexp.

Based on your outputs I’m fairly sure you’re testing with an unsupported version.

from path-to-regexp.

It looks like [email protected] (latest on npm) is using [email protected], see expressjs/express/package.json#L50 - Same as what's shown on http://forbeslindesay.github.io/express-route-tester/ but I see that this project is up to version 6.x already so express is extremely out of date with this dependency.

Sorry for not checking versions. I assumed they were using a relatively right version given their big note about using this project under https://expressjs.com/en/guide/routing.html#route-paths and even this project readme links the live demo as the Express route tester.

Express uses path-to-regexp for matching the route paths; see the path-to-regexp documentation for all the possibilities in defining route paths. Express Route Tester is a handy tool for testing basic Express routes, although it does not support pattern matching.

Trying to escape the inner parenthesis has no effect on getting the right grouping with that version 0.1.7,

• /:time(A\(foo)?B) -> Invalid path
• /:time(A(foo\)?B) -> Invalid path
• /:time(A\(foo\)?B) -> /^(?:\/(A\(foo\))?B)\/?$/i ❌

Edit: Actually in [email protected] from the Express route tester, /:time(A\(foo\)?B) -> /^\/((?:A\\(foo\\)?B))(?:\/(?=$))?$/i it has an extra \ added ❌. Will need to test in the latest [email protected] version.

It looks like the new mission is to post the issue in the express issue tracker and update path-to-regexp in express.

Thanks for checking in on all of this!

from path-to-regexp.

It looks like the new mission is to post the issue in the express issue tracker and update path-to-regexp in express.

You probably want to try using the Express 5 release instead of Express 4 for an updated path-to-regexp.

from path-to-regexp.

@dougwilson It looks like Express 5 (plus it's still in beta) doesn't use path-to-regexp anymore (at least directly)? Maybe through the [email protected] sub-dependency which uses [email protected]:

• pathToRegexp('/:time(A(foo)?B)') -> /^\/([^\/]+?)\((?:A(foo))?B\)(?:\/)?$/i ❌
• pathToRegexp('/:time(A\(foo\)?B)') -> /^\/([^\/]+?)\((?:A(foo))?B\)(?:\/)?$/i ❌ (same as not escaping enough)
• pathToRegexp('/:time(A\\(foo\\)?B)') -> /^\/(A\\(foo\\)?B)(?:\/)?$/i ❌ (close but extra \)

With the latest [email protected]:

• pathToRegexp('/:time(A(foo)?B)') -> TypeError: Capturing groups are not allowed at 8 ❌
  • Maybe the use case of having something optional in the middle is not supported since capture groups are being complained about
• pathToRegexp('/:time(A\(foo\)?B)') -> TypeError: Capturing groups are not allowed at 8 ❌ (same as not escaping enough)
• pathToRegexp('/:time(A\\(foo\\)?B)') -> /^(?:\/(A\(foo\)?B))[\/#\?]?$/i ❌

@blakeembrey What's the correct input I should expect to work? What version are you seeing it work in?

from path-to-regexp.

The latest version is the correct behavior.

from path-to-regexp.

To be clearer too, none of these examples have extra backslashes either. I think you’re interpreting the JS strings incorrectly.

Since backslash is a special character you need another backslash to escape it. That’s why you’re seeing two when viewed as a string.

from path-to-regexp.

@blakeembrey Can you give an example?

I feel like I've tried all of variations for escaping the backslash (even 3 and 4 backslashes now)

from path-to-regexp.

You aren’t allowed to use capturing groups within the regex section of the string for this library. It’s exactly what the latest version says which I meant is the correct behavior.

I’m kind of guessing that’s what you’re trying to do?

from path-to-regexp.

You can make it a non-capturing group if that’s acceptable for your use case.

from path-to-regexp.

You can make it a non-capturing group if that’s acceptable for your use case.

Thanks for noting this caveat! Works perfect in [email protected] 👌

• pathToRegexp('/:time(A(?:foo)?B)') -> /^(?:\/(A(?:foo)?B))[\/#\?]?$/i ✅

And to note how it behaves in other versions (relevant for express):

[email protected]:

• pathToRegexp('/:time(A(?:foo)?B)') -> SyntaxError: Invalid regular expression: /^\/([^\/]+?)\((?:A(?\:foo))?B\)(?:\/)?$/: Invalid group ❌
• Can't get workarounds or other variations to work

[email protected]:

• pathToRegexp('/:time(A(?:foo)?B)') -> /^(?:\/(A(?:foo))?B)\/?$/i ❌
• pathToRegexp('/:time(A((?:(?:foo))?)B)') -> /^\/(?:(A((?:(?:foo)))?)B)\/?$/i ✅
  • The workaround mentioned in the issue description of wrapping in an extra capture group also works with a non-capture group

from path-to-regexp.