Implement URL.from #28482

viktor-ku · 2019-06-30T15:44:28Z

It should substitute legacy url.format with respect
to the new URL API.

Based on #25099

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

It should substitude legacy `url.format` with respect to the new URL API. Based on discussion: #25099

vsemozhetbyt

Thank you!
Just some possible doc nits.

doc/api/url.md

joyeecheung

I don't think we should invent non-standard extensions on the URL class - if we want to provide a convenient method (even just as an experiment), we could put it directly on url instead.

mscdex · 2019-06-30T16:36:06Z

lib/internal/url.js

+
+    let flags = 0;
+
+    if (


What if we instead wrote this more like we do in the existing codebase?:

if (protocol === 'file:' || protocol === 'https:' || protocol === 'wss:' || protocol === 'http:' || protocol === 'ftp:' || protocol === 'ws:' || protocol === 'gopher:') { flags |= URL_FLAGS_SPECIAL; } else if (!host) { flags |= URL_FLAGS_CANNOT_BE_BASE; }

What do you mean? Where would be such example?

Example of what it would look like? I provided one above.

lib/internal/url.js

viktor-ku · 2019-07-02T08:00:13Z

@joyeecheung you have the point and it is valid. But URL.from returns a standard URL instance. It even takes an object that represents URLContext. And I think URL.from is fine just because of that reason.

joyeecheung · 2019-07-02T08:43:40Z

@viktor-ku It would make sense if we controlled the URL interface, but URL comes from the URL standard and is placed in the global object. Adding a non-standard helper to URL just creates platform disparity without obvious gains, and it would make the API unnecessarily confusing if the upstream does add a similar way to contrust URL out of an object but with a different API. We wouldn't have that kind of risk if the method is just on url, the only downside is that it might be slightly less intuitive but then that is subjective - you can argue that having a non-standard API that only Node.js has (and cannot be found in the URL standard or places like MDN) is also unintuitive.

test/parallel/test-whatwg-url-from.js

viktor-ku · 2019-07-08T16:23:14Z

Just a quick update: URL.from wasn't welcomed to whatwg/url spec, so I will rename URL.from to something like createURL and also address feedback I got here. Probably on the weekend.

Any thought on how to name such function?

@joyeecheung

Fishrock123 · 2019-07-10T15:15:16Z

Just a quick update: URL.from wasn't welcomed to whatwg/url spec

Could you link to that conversation? That seems silly. Web browsers also suffer from this.

test/parallel/test-whatwg-url-from.js

devsnek · 2019-07-10T15:25:21Z

@Fishrock123 previous convos: whatwg/url#354 and #12682

right now URL is designed such that it can only ever represent an unambiguous absolute url. The current minimal possible output from this API (':') is very far from what URL objects are supposed to be able to represent. In both discussions above, it was recommended that node introduce a new api (not URL) for relative urls.

jasnell · 2019-07-15T14:33:05Z

I'm +1 on adding the URL.from() function but the implementation can be greatly simplified here using something like:

URL.from = function(args) {
  return Object.assign(new URL(`${args.protocol||'http:'}//${args.host}||'example.com'}`), args);
}

Albeit with argument checking added.

devsnek · 2019-07-15T15:03:49Z

@jasnell needs an host too, just new URL('http:') is an error.

jasnell · 2019-07-15T15:11:19Z

heh, yeah, I'd accidentally omitted that in the example above. Will fix!

silverwind · 2019-07-15T15:22:51Z

Should probably throw if protocol or host are absent? Or document the example values, lol.

jasnell · 2019-07-15T16:14:55Z

I think throwing would be appropriate if those aren't available. A number of different scenarios may need to be accounted for depending on the protocol that is given (my example above is an oversimplification). For instance, if the protocol is mailto:, then we shouldn't look for host, we should look for pathname... e.g.

Object.assign(new URL('mailto:bob@example.com'), { pathname: 'sally@example.org' })

We will essentially need to special case known special URL protocols and treat unknown ones generically, but doing so still simplifies the implementation relative to this PR

viktor-ku · 2019-07-16T16:34:55Z

Let me respectfully disagree on that one. I know that it will simplify implementation, but why would you need to parse the entire string when you already know parts of the URL like host, protocol etc? I want to make this function perform well! And yes, I'd like to add as minimal validation as possible so it would be users job to know what they want.

So I am leaving URL.from then 👍

I will restrict passing anything expect of {} and it will be their job to build what they need. Perhaps, one can write a complex 3rd party library that can perform all kinds of validations, but those who know exactly what they need won't be punished.

viktor-ku · 2019-07-27T15:17:37Z

PR has been updated!

vsemozhetbyt · 2019-07-27T15:22:13Z

doc/api/url.md

+```
+
+All properties are optional in case you want to build your `URL` object
+gradually (note that you have to pass an empty object in this case anyway)


Nit:

Suggested change

gradually (note that you have to pass an empty object in this case anyway)

gradually (note that you have to pass an empty object in this case anyway).

vsemozhetbyt · 2019-07-27T15:23:12Z

doc/api/url.md

@@ -1323,6 +1362,12 @@ console.log(myURL.origin);
 [`url.format()`]: #url_url_format_urlobject
 [`url.href`]: #url_url_href
 [`url.parse()`]: #url_url_parse_urlstring_parsequerystring_slashesdenotehost
+[`url.protocol`]: #url_url_protocol


Nit: bottom references are usually sorted in ASCII order.

tjconcept · 2020-04-16T14:49:03Z

What about hostname?

url.format({protocol: 'http', hostname: '::1'})
// http://[::1]/

viktor-ku · 2020-07-30T13:21:13Z

I will be honest, I was, and still is pretty confused about what to do with this. Opinions have greatly divided amongst reviewers and at this point I really don't know how (and if) to proceed. Closing for now, because of that.

jasnell · 2020-07-30T23:39:41Z

Thank you for the work on this @viktor-ku ... Yeah, can definitely understand the confusion, I think we're still working through exactly the right thing to do with this.

url,doc: implement URL.from

68de7d4

It should substitude legacy `url.format` with respect to the new URL API. Based on discussion: #25099

vsemozhetbyt reviewed Jun 30, 2019

View reviewed changes

doc/api/url.md Outdated Show resolved Hide resolved

doc/api/url.md Outdated Show resolved Hide resolved

doc/api/url.md Outdated Show resolved Hide resolved

doc/api/url.md Outdated Show resolved Hide resolved

joyeecheung requested changes Jun 30, 2019

View reviewed changes

mscdex reviewed Jun 30, 2019

View reviewed changes

devsnek reviewed Jun 30, 2019

View reviewed changes

lib/internal/url.js Show resolved Hide resolved

addaleax added semver-minor PRs that contain new features and should be released in the next minor version. url Issues and PRs related to the legacy built-in url module. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Jul 1, 2019

This was referenced Jul 2, 2019

Implement URL.from helper whatwg/url#443

Closed

Consider adding a constructor to make a URL from parts whatwg/url#354

Closed

silverwind reviewed Jul 3, 2019

View reviewed changes

test/parallel/test-whatwg-url-from.js Show resolved Hide resolved

devsnek reviewed Jul 10, 2019

View reviewed changes

test/parallel/test-whatwg-url-from.js Outdated Show resolved Hide resolved

viktor-ku added 4 commits July 27, 2019 17:06

doc: place docs before instance methods

6d4126c

docs: add references

68924d9

doc, url: restrict the input to from

361115d

doc: clearify the output

49a5fa0

vsemozhetbyt reviewed Jul 27, 2019

View reviewed changes

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:51

gillesdemey mentioned this pull request Oct 6, 2019

Remove require('url') in favor of URL feross/simple-get#53

Open

SukkaW mentioned this pull request Nov 9, 2019

url_for helper doesn't work for mailto link hexojs/hexo-util#122

Closed

BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19

jasnell added the stalled Issues and PRs that are stalled. label Jul 7, 2020

viktor-ku closed this Jul 30, 2020

pmmmwh mentioned this pull request Dec 30, 2020

fix: ipv6 localhost pmmmwh/react-refresh-webpack-plugin#288

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement URL.from #28482

Implement URL.from #28482

viktor-ku commented Jun 30, 2019

vsemozhetbyt left a comment

joyeecheung left a comment

mscdex Jun 30, 2019

viktor-ku Jul 2, 2019

mscdex Jul 27, 2019

viktor-ku commented Jul 2, 2019

joyeecheung commented Jul 2, 2019 •

edited

Loading

viktor-ku commented Jul 8, 2019

Fishrock123 commented Jul 10, 2019

devsnek commented Jul 10, 2019 •

edited

Loading

jasnell commented Jul 15, 2019 •

edited

Loading

devsnek commented Jul 15, 2019

jasnell commented Jul 15, 2019

silverwind commented Jul 15, 2019 •

edited

Loading

jasnell commented Jul 15, 2019

viktor-ku commented Jul 16, 2019

viktor-ku commented Jul 27, 2019

vsemozhetbyt Jul 27, 2019

vsemozhetbyt Jul 27, 2019

tjconcept commented Apr 16, 2020

viktor-ku commented Jul 30, 2020

jasnell commented Jul 30, 2020

	gradually (note that you have to pass an empty object in this case anyway)
	gradually (note that you have to pass an empty object in this case anyway).

Implement URL.from #28482

Implement URL.from #28482

Conversation

viktor-ku commented Jun 30, 2019

Checklist

vsemozhetbyt left a comment

Choose a reason for hiding this comment

joyeecheung left a comment

Choose a reason for hiding this comment

mscdex Jun 30, 2019

Choose a reason for hiding this comment

viktor-ku Jul 2, 2019

Choose a reason for hiding this comment

mscdex Jul 27, 2019

Choose a reason for hiding this comment

viktor-ku commented Jul 2, 2019

joyeecheung commented Jul 2, 2019 • edited Loading

viktor-ku commented Jul 8, 2019

Fishrock123 commented Jul 10, 2019

devsnek commented Jul 10, 2019 • edited Loading

jasnell commented Jul 15, 2019 • edited Loading

devsnek commented Jul 15, 2019

jasnell commented Jul 15, 2019

silverwind commented Jul 15, 2019 • edited Loading

jasnell commented Jul 15, 2019

viktor-ku commented Jul 16, 2019

viktor-ku commented Jul 27, 2019

vsemozhetbyt Jul 27, 2019

Choose a reason for hiding this comment

vsemozhetbyt Jul 27, 2019

Choose a reason for hiding this comment

tjconcept commented Apr 16, 2020

viktor-ku commented Jul 30, 2020

jasnell commented Jul 30, 2020

joyeecheung commented Jul 2, 2019 •

edited

Loading

devsnek commented Jul 10, 2019 •

edited

Loading

jasnell commented Jul 15, 2019 •

edited

Loading

silverwind commented Jul 15, 2019 •

edited

Loading