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

WIP: http: replace url.parse with WHATWG URL parser #20288

Closed
wants to merge 3 commits into from

Conversation

TimothyGu
Copy link
Member

@TimothyGu TimothyGu commented Apr 25, 2018

Alternative to #20270. Maintains compatibility in case of e.g. http://xn--1ug.example/, which is not a valid URL but http accepted it anyway.

Also makes urlToOptions safe against any tampering of URL.

This is a WIP; the following parts are not yet done:

  • Tests
  • Same change in HTTPS
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Apr 25, 2018
@mscdex mscdex added the wip Issues and PRs that are still a work in progress. label Apr 25, 2018
Some previously supported (but strictly invalid) URLs were accepted through the
[`http.request()`][] and related APIs. This behavior is now deprecated. It is
recommended that one passes a [`URL`][] object to the API as the first argument
which ensures URL validity.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm... the wording here I think can be improved.

### DEP00XX: http.request() and http.get() support for invalid URLs

Some previously supported (but strictly invalid) URLs were accepted through the
[`http.request()`][] and [`http.get()`][] APIs because those were accepted by the
legacy `url.parse()` API. The `http.request()` and request APIs now use the WHATWG
URL parser that requires strictly valid URLs. Passing an invalid URL is deprecated
and support will be removed in the future.

I specifically don't want to get in to recommending that users pass the URL object directly into the api as the URL spec itself recommends against that. Let's keep encouraging passing in the URLs as strings and put more attention on the underlying change that is prompting the deprecation.

@jdalton
Copy link
Member

jdalton commented Apr 26, 2018

Why open this PR while #20270 is still being worked on and not obstructed? 😕

It's kinda like shouting over someone in a conversation.
I'd let the new contributor contribute without pushing past them so soon.

@TimothyGu
Copy link
Member Author

@jdalton As I mentioned in #20270, I had these commits in my branch already even before that PR was opened. Instead of letting these commits go stale I opened a PR so that more people can see it. I do not have time to finish this PR (hence the WIP tag) and have no intention to let this supersede the original PR.

@TimothyGu
Copy link
Member Author

Changes incorporated to #20270. Closing.

@TimothyGu TimothyGu closed this Apr 27, 2018
@TimothyGu TimothyGu deleted the http-url branch April 27, 2018 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants