Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

faster url parser #6788

Closed
jonathanong opened this issue Jan 1, 2014 · 6 comments
Closed

faster url parser #6788

jonathanong opened this issue Jan 1, 2014 · 6 comments

Comments

@jonathanong
Copy link

https://github.com/petkaantonov/urlparser

@petkaantonov said you guys won't merge this in core, but i don't see a reason not to, especially when you guys are/will be focusing on performance improvements, there are no regressions, there are more tests, and the code is easier to read.

last thing i want are PRs for faster node modules (and the consequential maintenance burden) in every repo when node should be improving performance themselves.

related: senchalabs/connect#986

@stefanpenner
Copy link

Getting this in core would be ideal. For future reference I am quite interesting in knowing what (if any) the blockers and concerns turn out to be.

@tjfontaine
Copy link

To quote my original senchalabs/connect#986 (comment)

For posterity, at the moment I am not interested in doing a substantial rewrite of a core module before 0.12 or for 1.0 which will be coming immediately afterward. That doesn't mean another team member won't want to champion it.

It is a scary proposition for a relatively new module to replace particularly important functionality. That's why I advocate leaving it as an external module for now, where people can opt into using it and it can grow and mature on its own without node standing in its way.

As it grows in popularity and need, it can be integrated into node (as other modules have transitioned historically). Or such time that a core team member has the time, motivation, and priority to give the module the attention it deserves, and integrate it that way.

In the meantime know that we aren't unsympathetic to your needs, but it is certainly possible to work around that for now without requiring a change to node proper.

@jonathanong
Copy link
Author

okay cool. thanks for putting it on a milestone.

here's a hacky PR to replace node's url parser globally: petkaantonov/urlparser#2

@jbergstroem
Copy link
Member

How about baking a patch that lands additional tests for starters?

@jonathanong
Copy link
Author

ask @petkaantonov. i'm not going to make a PR unless i know someone's going to look at it. there's already 169 PRs that have yet to be merged or rejected. i'm not going to add another one.

@jasnell
Copy link
Member

jasnell commented May 28, 2015

Given the excellent work that @petkaantonov has been doing on the io.js side, significant performance improvements will be landing in the converged stream. We can decide whether or not we want to backport those here once that work is complete.

@jasnell jasnell closed this as completed May 28, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants