Add named capturing groups to regexpToRegexp method

[damiankuriata]

I was looking into fixing issues raised on express repository #4277.

I modified regular expression in regexpToRegexp to match not only ( but also (?<*> ^{probably regexp could be better :)}.

No change with normal regexp matching, router.get(/\/regex\/(.+)/) -> localhost/regex/testData will result in {"0":"testData"}.

With named capturing groups, router.get(/\/regex2\/(?<groupname>.+)/) -> localhost/regex2/testData will result in {"groupname":"testData"}.
Mixed matched with named capturing groups, router.get(/\/(.+)\/(?<groupname>.+)\/(.+)/) -> localhost/testRegex/testData/extraStuff" will result in {"0":"testRegex","2":"extraStuff","groupname":"testData"}.

All tests are passing but I didn't add new tests to cover this functionality yet.
Would be great to get some feedback from you guys if I'm even looking in good place or is it worth working on it.

[blakeembrey]

This looks really great to me, thanks! From my understanding this will work in all current projects because the named group doesn't remove it from the array of RegExp results (e.g. this code depends on the position of keys in the result:

path-to-regexp/src/index.ts

Line 405 in 98ab888

const key = keys[i - 1];

). If you want to add a couple of tests around it we should be good to merge this.

For getting this feature into the current version of Express, you'll probably need to make this PR against an older branch here: https://github.com/pillarjs/path-to-regexp/blob/0.1.x/index.js

However, I believe this it can land in the next Express.js release with the current implementation.

[blakeembrey]

Suggested change

	const groups = path.source.match(/\((\?<.*?>)?(?!\?)/g);
	const groups = path.source.match(/\((?:\?<(.*?)>)?(?!\?)/g);

Something like this will resolve the comment you have below.

[damiankuriata]

Yeah, non-capturing groups, I tried that. Had no luck so I'm guessing, I made some mistake :)

Will try this :)

[damiankuriata]

Something like this will resolve the comment you have below.

That didn't work

$ curl http://localhost:1337/testRegex/testData/extraStuff -w "\n"
{"0":"testRegex","2":"extraStuff","(?<groupname>":"testData"}

[blakeembrey]

Oh yeah, that makes sense. It's happening with .match which is only getting the matches and not the groups. We probably need to use regexp.exec in a loop instead.

There's also https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/matchAll but it's not supported on earlier node.js versions or browsers.

[damiankuriata]

Using exec worked :)

Looping through exec results doesn't look nice tho.
Any idea how to make it nicer?

[damiankuriata]

Test is failing on node v8 as Named capture groups are behind '--harmony' runtime flag.

[blakeembrey]

@DamianKu You'll probably need to put the tests behind a version flag by checking the node.js version in the test suite.

[damiankuriata]

@DamianKu You'll probably need to put the tests behind a version flag by checking the node.js version in the test suite.

@blakeembrey
How I would go about doing that?
Do you have any example in other your projects that I could investigate? :)

[blakeembrey]

How I would go about doing that?

You can read the node.js version here: https://nodejs.org/api/process.html#process_process_version. And use something like https://www.npmjs.com/package/semver to compare whether you want to run it. Put it within an if condition to define the test.

Here's an example (albeit using TypeScript): https://github.com/TypeStrong/ts-node/blob/866bce6a175ec124fe62f269b22e91c92b40f875/src/index.spec.ts#L137

[damiankuriata]

Hah OK 😁 I'm aware of that. I don't know why I was thinking that you meant something more elaborate

…

On Tue, 14 Jul 2020, 02:16 Blake Embrey, ***@***.***> wrote: How I would go about doing that? You can read the node.js version here: https://nodejs.org/api/process.html#process_process_version. And use something like https://www.npmjs.com/package/semver to compare whether you want to run it. Put it within an if condition to define the test. Here's an example (albeit using TypeScript): https://github.com/TypeStrong/ts-node/blob/866bce6a175ec124fe62f269b22e91c92b40f875/src/index.spec.ts#L137 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#225 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AEE7BAKDB6CZDWQ7RT4OIT3R3OWW5ANCNFSM4OKTOBNA> .

[damiankuriata]

I added the semver check after the initialization of TEST variable.

If you prefer I can do ternary in TEST like so:

const TESTS: Test[] = [
    ...,

    ...(gte(process.version, "10.0.0")
    ? [
        [...test...],
        [...test...]
      ]
    : ([] as any)),
    
    ...,
];

[blakeembrey]

It would probably be good to stay consistent with the behavior in string tokens. In that method, we're only incrementing the counter each non-named group:

path-to-regexp/src/index.ts

Line 189 in 8b0ae94

name: name || key++,

.

That would mean you probably want execResult[1] || index++. We should add a mixed test of named and unnamed groups to make sure it doesn't break.

[damiankuriata]

OK will do that.

Does it mean that this /\/(.+)\/(?<groupname>.+)\/(.+)/ called with /testRegex/testData/extraStuff should result in {"0":"testRegex","1":"extraStuff","groupname":"testData"} ?

[blakeembrey]

Thanks @DamianKu! This looks great.