search

Rob--W / proxy-from-env

A Node.js library to get the proxy URL for a given URL based on standard environment variables (http_proxy, no_proxy, ...).
MIT License
52 stars 19 forks source link

WHATWG URL compliance #4

Open stevenvachon opened 7 years ago

stevenvachon commented 7 years ago

via universal-url

Rob--W commented 7 years ago

Why?

stevenvachon commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

Rob--W commented 7 years ago

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 commented 7 years ago

The specification uses TR46 to parse IDNA host names.

Benchmarks: https://github.com/nodejs/node/pull/10678#issuecomment-272607729

silverwind commented 2 years ago

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