Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider alternatives to path to regex pattern #42

Closed
jasnell opened this issue Dec 18, 2020 · 6 comments
Closed

Consider alternatives to path to regex pattern #42

jasnell opened this issue Dec 18, 2020 · 6 comments

Comments

@jasnell
Copy link

jasnell commented Dec 18, 2020

Great work so far. Happy to see progress here. Just one quick thing I'd like to point out is that the regex approach here certainly is not the fastest. I haven't read through everything in detail get so forgive me if I've missed something. I wanted to at least point towards the radix based approach that the fastify framework uses for parsing and matching https://github.com/delvedor/find-my-way.

@wanderview
Copy link
Member

wanderview commented Dec 18, 2020

Our goal so far has been to focus on ergonomics over performance. We've taken path-to-regexp's popularity as a signal of good ergonomics without significant performance issues.

Looking at the benchmarks from the link above it looks like the performance difference is in the microsecond range. This seems unlikely to be significant in browser use cases. I will admit, though, it could be more important in server environments. Maybe this is an area where browser and server use cases diverge.

I'll take a closer look after I get back from holiday in January. Thanks.

@wanderview
Copy link
Member

There might be some overlap with our plans to restrict what kind of patterns can be used for some web platform apis like service workers. Those restrictions are partially motivated by performance.

@jasnell
Copy link
Author

jasnell commented Dec 19, 2020

The key for server side cases would just be flexibility in the implementation. That is, I'd prefer the api not to bake in any assumptions or requirements that regexp is used under the covers.

@blakeembrey
Copy link

This is a good call out, but worth noting the only part in path-to-regexp assuming regex patterns are optional patterns within the match, e.g. /:foo(\d+). Those could be an optional feature, or omitted entirely for URLPattern support.

@wanderview
Copy link
Member

Or optimizations are triggered if custom regexp groups are not used in the pattern.

@wanderview
Copy link
Member

I'm going to close this one for now. As discussed above, optimizations can be handled as an implementation detail. Also, we chose path-to-regexp for ecosystem reasons due to its existing popular usage providing a well lit path to follow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants