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

WHATWG URL compliance #4

Open
stevenvachon opened this issue May 30, 2017 · 7 comments · May be fixed by #14
Open

WHATWG URL compliance #4

stevenvachon opened this issue May 30, 2017 · 7 comments · May be fixed by #14

Comments

@stevenvachon
Copy link

via universal-url

@Rob--W
Copy link
Owner

Rob--W commented May 30, 2017

Why?

@stevenvachon
Copy link
Author

stevenvachon commented May 30, 2017

Because it's only going to get more popular with time. http/https modules support them in Node 8.x. They also parse at least 2x faster.

@Rob--W
Copy link
Owner

Rob--W commented May 30, 2017

My library is already compatible with WHATWG URLs, the exported function behaves identical regardless of whether the input is a WHATWG URL or a legacy (Node.js) URL. The return value is a string. If the consumer really wants to have a URL, it is trivial to do new URL(returnValueHere) to obtain a URL.

So, I don't see a benefit in adding "universal-url" as a dependency.

@stevenvachon
Copy link
Author

I was referring to the internal use of url.parse(). It's better to use the specification parser.

@Rob--W
Copy link
Owner

Rob--W commented May 30, 2017

I was referring to the internal use of url.parse(). It's better to use the specification parser.

Better in what sense? I don't see a functional difference between the two parsers in my use case. I only use scheme (.protocol), host name (inferred from .host) and port (.port) properties of (parsed) URLs.

There must be a compelling reason before I add new dependencies. The only potential advantage that you've mentioned so far is a "at least 2x faster". I don't attribute much value to this claim, not only because of the lack of benchmarks, but mainly because the existing implementation is already sufficiently fast and unlikely to be a bottleneck for performance issues.

@stevenvachon
Copy link
Author

stevenvachon commented May 30, 2017

The specification uses TR46 to parse IDNA host names.

Benchmarks: nodejs/node#10678 (comment)

@silverwind
Copy link

Should probably switch the legacy url to use URL, present as global since Node.js 10.0.0.

orgads added a commit to orgads/proxy-from-env that referenced this issue Nov 17, 2021
@orgads orgads linked a pull request Nov 17, 2021 that will close this issue
orgads added a commit to orgads/proxy-from-env that referenced this issue Jun 11, 2024
orgads added a commit to orgads/proxy-from-env that referenced this issue Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants