fix: ignore escaped parentheses when counting capture groups

[Hashim1999164]

Fixes #204

The problem

When a route is a RegExp, Layer scans the pattern source to build the list of param
keys. The scan regexp treats every ( as the start of a capture group, including a
literal paren that has been escaped with a backslash.

So a route like:

router.route(/^\/tenant\(v\d+\)-(?<tenant>[a-z]+)-(?<user>[a-z]+)$/)

registers three keys (0, tenant, user) while the RegExp only has two real
groups. The names then shift by one and req.params comes back as
{ 0: 'acme', tenant: 'boss' } instead of { tenant: 'acme', user: 'boss' }.

The fix

Add \\. as a leading alternative in MATCHING_GROUP_REGEXP so any escape sequence
is consumed as a single match, and skip those matches in the key-scanning loop.

I tried the shorter (?<!\\) lookbehind first, but it is wrong for a pattern that
contains an escaped backslash immediately followed by a real group, for example
/\\(\d+)/. The lookbehind sees the backslash before ( and skips a group that
actually exists, which shifts the keys the other way and makes a working route throw.
Consuming escapes with an alternation handles that case correctly because the \\
pair is eaten before the scanner reaches the (.

Out of scope: a paren inside a character class such as /[(]/ is still miscounted.
That is a separate pre-existing issue and fixing it properly needs character class
tracking, so I left it alone to keep this change small.

Test plan

• Added a regression test in test/route.js under the named capture group describe
  block, covering a route with an escaped literal paren plus two named groups.
• Confirmed the new test fails on main and passes with the fix.
• Full suite green: 695 passing.
• npm run lint (standard) clean.

[krzysdz]

I'm pretty sure this is doable with lookbehind and no other changes to logic, at least if character classes are ignored (the [(] case).

[Hashim1999164]

Good point on lookbehind. I kept the \\.|( alternate so any escaped character is skipped before we treat ( as a capture group start. That covers \( without changing the capture-group branch itself.

A lookbehind like (?<!\\)\( would also work for the escaped-paren case. I avoided it mainly to stay compatible with older JS engines that lack lookbehind, since this package still supports a fairly wide Node range. Happy to switch to lookbehind if maintainers prefer that style and are fine requiring engines that support it.

Character classes like [(] are still out of scope either way, same as you noted.

[blakeembrey]

Seems reasonable to me. The lookbehind might not work everywhere because you could have an escaped escape like \\\(.

[krzysdz]

Seems reasonable to me. The lookbehind might not work everywhere because you could have an escaped escape like \\\(.

But one like this should work¹ (beginning of string or character other that \, followed by an even (incl. 0) number of \ characters):

/(?<=(?:^|[^\\])(?:\\\\)*)\((?:\?<(.*?)>)?(?!\?)/g

Footnotes

1. By should work I mean that I have tested it and it works. ↩

[krzysdz]

And by the way escaped backslashes probably can't be found in real world paths, because most clients normalise \ in path to / (curl doesn't). I tried to write tests for this case, but node:url and URL normalise them, breaking the tests (see krzysdz/router@1f1e9a9).

[Hashim1999164]

Yeah fair point. Most clients will turn backslashes into slashes before the path even hits the router, so the weird \\\( style paths are probably not something we need to worry about in practice.

I looked at your test commit too. Makes sense that node:url and URL normalize things and make that hard to test cleanly.

I am fine keeping this version as is since blakeembrey already approved it and it fixes the real bug from #204. If you or the maintainers still want the lookbehind regex instead I can swap it, but I would rather not expand scope here unless someone asks for it.