doc: add basic documentation for WHATWG URL API#10620
doc: add basic documentation for WHATWG URL API#10620jasnell wants to merge 2 commits intonodejs:masterfrom
Conversation
doc/api/url.md
Outdated
There was a problem hiding this comment.
define experimental, perhaps by including another > Stability: ... stanza here.
doc/api/url.md
Outdated
There was a problem hiding this comment.
What does this actually do? Does it cause all the other properties to be re-evaluated as if the myURL had been constructuted from this new URL string?
doc/api/url.md
Outdated
doc/api/url.md
Outdated
There was a problem hiding this comment.
Why is it so specific about Unicode seialization here, and not in the other string properties? This one is different?
There was a problem hiding this comment.
That one is specificly pointed out in the spec https://url.spec.whatwg.org/#dom-url-origin
The origin attribute’s getter must return the Unicode serialization of context object’s url’s origin. [HTML]
It returns the Unicode rather than the ASCII serialization for compatibility with HTML’s MessageEvent feature. [HTML]
doc/api/url.md
Outdated
There was a problem hiding this comment.
if delete url.port, does it become "http://example.org"?
There was a problem hiding this comment.
To be more precise: it has no effect in sloppy mode and throws in strict mode. I think it deserves a note somewhere that to remove writable properties, one must set an empty value.
There was a problem hiding this comment.
hmm.. it does not appear to throw in strict mode... always appears to return true but have no effect.
'use strict';
const URL = require('url').URL;
const u = new URL('http://example.org:123/abc');
console.log(delete u.port);
console.log(u.href);There was a problem hiding this comment.
That's because port is not a property on u, but rather u.[[Prototype]] (i.e. URL.prototype), so delete u.port will not affect the object.
However, since URL.prototype.port is configurable, you can actually delete URL.prototype.port in both strict and sloppy modes.
There was a problem hiding this comment.
I see. Sorry for the misunderstanding
doc/api/url.md
Outdated
There was a problem hiding this comment.
what if you delete it? actually, for all the properties, what happens if they are deleted?
also, what happens if you assign an invalid value? does it throw, like it does in construction?
There was a problem hiding this comment.
Could add a hint about all the attributes of properties https://heycam.github.io/webidl/#es-attributes at the top or somewhere (all enumerable: true and configurable: false for URL properties, writable depends on read-only-ness).
There was a problem hiding this comment.
Potentially but we do not typically go into that level of detail for most Node.js properties and the original spec is already pretty well defined.
doc/api/url.md
Outdated
There was a problem hiding this comment.
serialized, as opposed to ...?
There was a problem hiding this comment.
As opposed to searchParams
doc/api/url.md
Outdated
There was a problem hiding this comment.
should that be linked to the def'n of URLSearchParams?
doc/api/url.md
Outdated
There was a problem hiding this comment.
Why are they here, then, are they going to be deprecated and removed?
There was a problem hiding this comment.
same question for the APIs below
There was a problem hiding this comment.
No, these are added as part of the new experimental implementation. They are just not part of the standard
232d199 to
d3adc47
Compare
|
Updated to address nits |
doc/api/url.md
Outdated
There was a problem hiding this comment.
Maybe worth pointing out that the additional escaped characters in url.parse (e.g. ") are no longer escaped (not necessarily here because that's common to a lot of properties).
doc/api/url.md
Outdated
There was a problem hiding this comment.
The delete statement would return true even though it has no effect. Could use a hint?
doc/api/url.md
Outdated
There was a problem hiding this comment.
Might be worth adding
myURL.href = 'http://你好你好';
// Prints http://xn--6qqa088eba/
// Notice the slash added at the end and the ASCII serialization
doc/api/url.md
Outdated
There was a problem hiding this comment.
Might be worth adding
const idnURL = new URL('http://你好你好');
console.log(idnURL.origin);
// Prints http://你好你好
doc/api/url.md
Outdated
There was a problem hiding this comment.
Worth pointing out it returns a string, not a number. Also setting it to the protocol's default port would result in a '' returned, that's different from what url.parse would do.
doc/api/url.md
Outdated
doc/api/url.md
Outdated
There was a problem hiding this comment.
Has a question mark at the beginning, ?123
doc/api/url.md
Outdated
There was a problem hiding this comment.
Could use a tip about it returning the same result as #href
|
Overall I think we should document the behaviors different from |
|
@joyeecheung ... definitely think that would be helpful but perhaps a guide would be better for that? In the meantime, I've address some of your comments! PTAL! |
doc/api/url.md
Outdated
There was a problem hiding this comment.
Any reason this comment is indented (and others below)?
There was a problem hiding this comment.
Just a style I generally prefer (and have used in other docs that show the expected output
doc/api/url.md
Outdated
There was a problem hiding this comment.
Not very sure about these, if they are not part of the public API, maybe we should avoid documenting them?
There was a problem hiding this comment.
They are not part of the WHATWG specification. They will be part of the Node.js API, however.
doc/api/url.md
Outdated
There was a problem hiding this comment.
Gets as a string, sets as either a string or a number.
|
@jasnell Yeah a guide would definitely be more suitable for this. I've submitted another review, if resolved then LGTM. |
|
I've added some specific details about the pct-encoding in the new implementation |
doc/api/url.md
Outdated
There was a problem hiding this comment.
Perhaps the @@iterator notation used in ES spec would be more concise? Or is this notation already used in the documentation?
There was a problem hiding this comment.
I don't believe there is much (or any) precedence for documenting @@iterator in our current docs given that few (if any) of our core APIs make use of it.
doc/api/url.md
Outdated
There was a problem hiding this comment.
There are also keys, values, and entries functions.
doc/api/url.md
Outdated
There was a problem hiding this comment.
I don't think "Returns undefined" needs to be explicitly stated…
doc/api/url.md
Outdated
There was a problem hiding this comment.
Actually, it returns null when the param cannot be found.
doc/api/url.md
Outdated
There was a problem hiding this comment.
Other places of the documentation seem to prefer
* Returns: {Array}76ca474 to
91d64d9
Compare
91d64d9 to
b836c24
Compare
doc/api/url.md
Outdated
There was a problem hiding this comment.
While at it, why don't you add an MDN link for the Iterator type to tools/doc/type-parser.js ? Then, you will be able to use {Iterator} with automatic linking.
doc/api/url.md
Outdated
There was a problem hiding this comment.
I'd explicitly mention that by definition, urlSearchParams[@@iterator] === urlSearchParams.entries. You can completely replace this description with the aliasing information (which has the benefit of not going out of sync), or make that info an additional paragraph – either is fine with me.
b836c24 to
6b6c374
Compare
|
Updated! |
|
Will land this tomorrow if there are further objections or comments. |
|
This haven't moved for a while. Pinging @Fishrock123 , are you OK with landing this now? |
|
Yeah, I intentionally left it open just in case. I'm planning on landing it today. |
PR-URL: #10620 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michal Zasso <targos@protonmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
PR-URL: #10620 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michal Zasso <targos@protonmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
|
Landed in 0c712b6 and 4757ddc |
PR-URL: nodejs#10620 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michal Zasso <targos@protonmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
PR-URL: nodejs#10620 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michal Zasso <targos@protonmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
PR-URL: nodejs#10620 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michal Zasso <targos@protonmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
PR-URL: nodejs#10620 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michal Zasso <targos@protonmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
PR-URL: nodejs#10620 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michal Zasso <targos@protonmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
PR-URL: nodejs#10620 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michal Zasso <targos@protonmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
PR-URL: nodejs#10620 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michal Zasso <targos@protonmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
PR-URL: nodejs#10620 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michal Zasso <targos@protonmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Notable changes: * crypto: * ability to select cert store at runtime (Adam Majer) #8334 * Use system CAs instead of using bundled ones (Adam Majer) #8334 * deps: * upgrade npm to 4.1.2 (Kat Marchán) #11020 * upgrade openssl sources to 1.0.2k (Shigeki Ohtsu) #11021 * doc: add basic documentation for WHATWG URL API (James M Snell) #10620 * process: add NODE_NO_WARNINGS environment variable (cjihrig) #10842 * url: allow use of URL with http.request and https.request (James M Snell) #10638 PR-URL: #11062
Notable changes: * crypto: * ability to select cert store at runtime (Adam Majer) #8334 * Use system CAs instead of using bundled ones (Adam Majer) #8334 * deps: * upgrade npm to 4.1.2 (Kat Marchán) #11020 * upgrade openssl sources to 1.0.2k (Shigeki Ohtsu) #11021 * doc: add basic documentation for WHATWG URL API (James M Snell) #10620 * process: add NODE_NO_WARNINGS environment variable (cjihrig) #10842 * url: allow use of URL with http.request and https.request (James M Snell) #10638 PR-URL: #11062
Add documentation for the WHATWG URL API
Checklist
Affected core subsystem(s)
doc,url