? in optional capture group isn't connected to group in output - named param

[MadLittleMods]

Expected goal

The goal is to make a route where foo is optional between A and B:

Bug

But using the following input gives a flawed output regex that groups A and foo together in the optional capture group.

Input:

/:time(A(foo)?B)

Output ❌:

/^(?:\/(A(foo))?B)\/?$/i

Workaround

Workaround input is to wrap the optional capture group in another capture group:

Input:

/:time(A((foo)?)B)

Output (works sufficiently) ✅:

/^(?:\/(A((foo))?)B)\/?$/i

---

Input and output tested on http://forbeslindesay.github.io/express-route-tester/

[blakeembrey]

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?

[blakeembrey]

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

[MadLittleMods]

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!

[dougwilson]

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.

[MadLittleMods]

@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?

[blakeembrey]

The latest version is the correct behavior.

[blakeembrey]

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.

[MadLittleMods]

@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)

[blakeembrey]

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?

[blakeembrey]

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

[MadLittleMods]

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