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

url: add experimental URLPattern #42133

Closed
wants to merge 2 commits into from
Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Feb 25, 2022

The start of a URLPattern implementation. Still very much a WIP.

Fixes: #40844

Signed-off-by: James M Snell jasnell@gmail.com

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. url Issues and PRs related to the legacy built-in url module. labels Feb 25, 2022
@bmeck
Copy link
Member

bmeck commented Feb 25, 2022

this would be nice for the policies implementation eventually regardless of public API status. right now it does a new URL('..') crawl that gets really weird once you get to the protocol level.

@Mesteery Mesteery linked an issue Feb 25, 2022 that may be closed by this pull request
@mcollina
Copy link
Member

As I said multiple times in the spec repo, this is not a good API for Node.js as it will eventually become an hot path in most servers code.

@jasnell
Copy link
Member Author

jasnell commented Feb 25, 2022

The API itself is Ok. The reg-ex based impl under the covers leaves lots of room for improvement. Given that this is a Web Platform API, I think there's good reason to provide an implementation but we definitely should put effort into optimizing the internals before it graduates from experimental

@mcollina
Copy link
Member

The only way to use this API is by iterating over an array of them :(. I'm not convinced this is a good API for Node.js.

@jasnell
Copy link
Member Author

jasnell commented Feb 26, 2022

That's true when using it as a router. There are plenty of use cases for using just a single pattern.

@mcollina
Copy link
Member

I'm not aware of other use cases than URL routing. What are those?

I would like to note that there are multiple solutions in the ecosystem that are significantly better than this API for server-side routing. Landing this in node will pass the wrong message.

This is also a new API and there is no pressure right now from the ecosystem to adopt it, so I think we can wait until we can promote an extension to this API that allows to scale.

@Jamesernator
Copy link

This is also a new API and there is no pressure right now from the ecosystem to adopt it, so I think we can wait until we can promote an extension to this API that allows to scale.

Could Node just provide a utility in say url that just provides a way to optimize a list of URLPattern? i.e. something like url.optimizePatternList([urlPattern1, urlPattern2]);

Like the entire URLPattern object was designed based on the already very popular path-to-regexp library, and given that the internals can essentially just be regexps wouldn't you effectively just be merging the regexps into a single state machine.

And isn't v8's regexp engine pretty fast already? In which case just doing something like pattern1|pattern2|...|patternN would allow v8 to make this regexp for you (some slight modification would need to be done to "tag" each pattern).

Signed-off-by: James M Snell <jasnell@gmail.com>
@Jamesernator
Copy link

The only way to use this API is by iterating over an array of them :(. I'm not convinced this is a good API for Node.js.

So Chrome just announced an intent to prototype a URLPatternList which is meant to solve precisely this issue.

@anonrig
Copy link
Member

anonrig commented Feb 19, 2023

@jasnell What was the reason for abandoning this?

@jasnell
Copy link
Member Author

jasnell commented Feb 19, 2023

Lack of time to finish it. Happy to have someone take it on

@debadree25
Copy link
Member

I can try attempting this, is only the TODOs in the code left, or are more spec features remaining to be implemented?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. needs-ci PRs that need a full CI run. tools Issues and PRs related to the tools directory. url Issues and PRs related to the legacy built-in url module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

implement URLPattern
8 participants