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 #239

Merged
merged 6 commits into from Mar 5, 2019
Merged

Whatwg url #239

merged 6 commits into from Mar 5, 2019

Conversation

@WesTyler
Copy link
Contributor

WesTyler commented Feb 28, 2019

Per #238 - this updates Wreck to leverage the WHATWG Url parser rather than the old Url.parse.

There are some breaking changes in how hosts and URIs are parsed between the two methods, especially when it comes to host headers containing the port and also to Unix socket connections. I wanted to keep this as a non-breaking update in Wreck, so there are some maybe-weird conditionals to handle parsing the uri using the old Url.parse for Unix sockets as well as some weird property mapping.

With regards to the last point (internals.applyUrlToOptions), this is due to the getters on the Symbol properties on the new Url.URL class. Just as a point of interest, this is the exact same strategy that Node.js itself is using internally in making a request instance here.

I'm assuming this is going to be a first pass, so please let me know what changes and simplifications you'd like to see. :)

WesTyler added 2 commits Feb 27, 2019
@WesTyler

This comment has been minimized.

Copy link
Contributor Author

WesTyler commented Feb 28, 2019

Looking at why Node 11 and current are failing on redirects with a different host.

[Edit] Node 11 combined the request object properties 'output', 'outputEncodings', and 'outputCallbacks' into a single object.

lib/index.js Show resolved Hide resolved
Copy link
Member

geek left a comment

Looks a lot better... do you think it will be easier to just do a breaking release and remove the old Url.parse?

lib/index.js Outdated Show resolved Hide resolved
lib/index.js Outdated Show resolved Hide resolved
lib/index.js Show resolved Hide resolved
@WesTyler

This comment has been minimized.

Copy link
Contributor Author

WesTyler commented Mar 1, 2019

do you think it will be easier to just do a breaking release and remove the old Url.parse

I actually don't think so, mostly because of the support for Unix sockets. I cannot for the life of me find a way to get Unix sockets to play nicely with the WHATWG parser.
Also, I intentionally left Url.parse in internals.resolveUrl since path parsing behaves a bit differently between the two and that uri flows into the WHATWG parser eventually anyways.

WesTyler added 2 commits Mar 1, 2019
lib/index.js Outdated Show resolved Hide resolved
@geek geek added this to the 14.2.0 milestone Mar 5, 2019
@geek
geek approved these changes Mar 5, 2019
@geek geek added the feature label Mar 5, 2019
@geek geek merged commit 8ebcbd3 into hapijs:master Mar 5, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.