Add WHATWG URL support #289

alextes · 2017-03-26T22:00:07Z

So I've gotten stuck implementing this.
The way I imagine things, this PR is a bad idea or I've been way overthinking the whole time. With this PR the nuances between the way Node handles a Node urlObject or whatwg urlObject would be lost. I tried to dig into the Node v8 nightly to confirm but couldn't find anything. I assumed this can't be done because this PR trivially supports whatwg-urls on any Node and I thought that wasn't possible.

The second suggestion was keeping the URL object intact and supporting whatwg-url on Node >= v8. This makes less sense the more I think about it. Some issues I can think of:

Any place you read from or write to the URL you'd have to be aware of which type you're dealing with. (If you normalize you're effectively doing what this PR is)
We currently support all of the http.request options, which we use to overwrite the URL object.
How does one pass node's http.request options that are not Node URL properties? If you have to serialize the whatwg-url in order to do that what's the point keeping it intact in the first place? The current Node implementation runs it through url.parse anyway. If that is going to stay the same in v8, this PR seems like an adequate solution.

I guess the core question I'm asking is: does it matter if you serialize a whatwg-url then parse it with node's url.parse or directly pass it to node? I can't tell from skimming the whatwg-url spec.

Note to self: if we're going with normalizing whatwg-url's take care of sneaking in auth.
Sidenote to self: dealing with features supported in some versions of Node is tricky 😓.

Fixes #228

sindresorhus · 2017-05-01T05:46:00Z

I think this is the correct approach. We just want to support WHATWG URLs now, not use them internally. Can you also document this?

sindresorhus · 2017-05-01T05:48:23Z

package.json

@@ -48,6 +48,7 @@
    "is-redirect": "^1.0.0",
    "is-retry-allowed": "^1.0.0",
    "is-stream": "^1.0.0",
+    "isurl": "^1.0.0-alpha3",


It's 1.0.0-alpha5 now

This one doesn't work 😓 .

// @stevenvachon

isURL.lenient(url)

sindresorhus · 2017-05-01T05:48:47Z

package.json

@@ -64,6 +65,7 @@
    "pem": "^1.4.4",
    "pify": "^2.3.0",
    "tempfile": "^1.1.1",
+    "whatwg-url": "^4.6.0",


And ^4.7.1

sindresorhus · 2017-05-04T02:58:22Z

@stevenvachon Can you review?

sindresorhus · 2017-05-04T02:59:00Z

@alextes Tests are failing

stevenvachon · 2017-05-04T03:45:25Z

test/arguments.js

@@ -1,3 +1,4 @@
+import {URL} from 'whatwg-url';


It might be more complete to use universal-url here so that the Node v7 URL is included in tests when available.

Sure. It'd be nice to test both the native and shim. I'm swapping it out. Just to make sure, how does the rest feel about going from a lib that's seen tons of use to something that's still in alpha?

stevenvachon · 2017-05-04T03:47:29Z

index.js

@@ -206,6 +207,10 @@ function normalizeArguments(url, opts) {
 		}
 	}

+	if (isURL(url)) {
+		url = urlParseLax(url.href);


There's no auth (username,password) check in this block like there is for string input.

Also, reparsing the url is not the best idea for performance.

stevenvachon · 2017-05-04T03:53:35Z

readme.md

@@ -62,7 +62,7 @@ Returns a Promise for a `response` object with a `body` property, a `url` proper

 Type: `string`, `object`

-The URL to request or a [`http.request` options](https://nodejs.org/api/http.html#http_http_request_options_callback) object.
+The URL to request as simple string, a [`http.request` options](https://nodejs.org/api/http.html#http_http_request_options_callback), or a [`WHATWG-URL`](https://nodejs.org/api/url.html#url_class_url).


This is more accurate:

, or a WHATWG URL.

alextes · 2017-05-04T22:50:12Z

Thanks for the feedback! Will update tomorrow 😄.

alextes · 2017-05-13T22:58:18Z

@sindresorhus @stevenvachon ready for review 😁!

stevenvachon · 2017-05-14T07:38:43Z

package.json

@@ -69,6 +70,8 @@
    "nyc": "^10.0.0",
    "pem": "^1.4.4",
    "pify": "^2.3.0",
+    "universal-url": "^1.0.0-alpha",


Alphabetical order.

Sorry 😅. It was late.

alextes changed the title ~~Add WHATWG-URL support~~ Add WHATWG-URL support (needs feedback) Mar 31, 2017

alextes changed the title ~~Add WHATWG-URL support (needs feedback)~~ Add WHATWG-URL support Apr 24, 2017

sindresorhus reviewed May 1, 2017

View reviewed changes

alextes force-pushed the whatwg-url branch from 41e1e65 to 5fe6a14 Compare May 4, 2017 00:39

Add a whatwg-url test

b1ce3be

alextes force-pushed the whatwg-url branch 2 times, most recently from dcb8677 to 28fe6d3 Compare May 4, 2017 01:25

stevenvachon reviewed May 4, 2017

View reviewed changes

alextes added 2 commits May 4, 2017 23:13

Serialize and parse whatwg-urls

1209e89

Document the option to use WHATWG-URLs

ec62095

alextes force-pushed the whatwg-url branch from 28fe6d3 to ec62095 Compare May 4, 2017 21:13

alextes added 5 commits May 14, 2017 00:47

Swap whatwg-url for universal-url

d1f01ab

Error on request url with auth

8beba9e

Use test functions over simple throwing in test

fe5ac67

Improve whatwg URL support documentation

a26e7f7

Merge branch 'master' into whatwg-url

c56354c

stevenvachon reviewed May 14, 2017

View reviewed changes

Correct dependency alphabetical order

7b8bdbc

stevenvachon approved these changes May 14, 2017

View reviewed changes

sindresorhus changed the title ~~Add WHATWG-URL support~~ Add WHATWG URL support May 15, 2017

sindresorhus merged commit f501fc2 into sindresorhus:master May 15, 2017

alextes deleted the whatwg-url branch May 15, 2017 19:17

This was referenced May 29, 2017

Update dependency got to version 7.x singapore/gl-got#26

Closed

Update dependency got to version 7.x renovatebot/renovate#223

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add WHATWG URL support #289

Add WHATWG URL support #289

alextes commented Mar 26, 2017 •

edited

Loading

sindresorhus commented May 1, 2017

sindresorhus May 1, 2017

alextes May 4, 2017

sindresorhus May 4, 2017

stevenvachon May 4, 2017

sindresorhus May 1, 2017

sindresorhus commented May 4, 2017

sindresorhus commented May 4, 2017

stevenvachon May 4, 2017

alextes May 13, 2017

stevenvachon May 4, 2017

stevenvachon May 4, 2017

stevenvachon May 4, 2017 •

edited

Loading

alextes commented May 4, 2017

alextes commented May 13, 2017

stevenvachon May 14, 2017

alextes May 14, 2017

Add WHATWG URL support #289

Add WHATWG URL support #289

Conversation

alextes commented Mar 26, 2017 • edited Loading

sindresorhus commented May 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented May 4, 2017

sindresorhus commented May 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenvachon May 4, 2017 • edited Loading

Choose a reason for hiding this comment

alextes commented May 4, 2017

alextes commented May 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alextes commented Mar 26, 2017 •

edited

Loading

stevenvachon May 4, 2017 •

edited

Loading