Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.2k
doc: discuss special protocol handling#22261
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot commented Aug 11, 2018
jasnell commented Aug 11, 2018
Please 👍 to fast-track |
Trott left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some non-blocking nits.
doc/api/url.md Outdated
| ##### Special Schemes | ||
| The WHATWG URL Standard considers a handful of URL protocol schemes to be | ||
| "special" in terms of how those are parsed and serialized. When a URL is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Italics instead of quotes around
special. those->they
doc/api/url.md Outdated
| ``` | ||
| However, changing from `http` to a hypothetical `fish` protocol does not | ||
| because the new protocol is not considered "special". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- No quotes around
special.
doc/api/url.md Outdated
| ##### Special Schemes | ||
| The WHATWG URL Standard considers a handful of URL protocol schemes to be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linking the text WHATWG URL Standard to the standard itself would be useful here. It's already linked in two other places in the doc, so it's just a matter of changing the markdown to [WHATWG URL Standard][].
Trott commented Aug 11, 2018
(Intentionally not approving fast-tracking here. There's enough new material here that it's probably worth not rushing the review process IMO and I'm unaware of any reason to rush landing it. I won't block fast-tracking but I'd ask people to consider carefully if this really needs to be fast-tracked.) |
Trott commented Aug 11, 2018
@jasnell You added the |
jasnell commented Aug 11, 2018
Feel free to push your suggested edits to the PR. |
Trott commented Aug 11, 2018
👍 Done. |
Fixes: #13523 PR-URL: #22261 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jon Moss <[email protected]>
jasnell commented Aug 11, 2018
Landed in 58cf409 (fast tracked with approval) |
Fixes: #13523 PR-URL: #22261 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jon Moss <[email protected]>
bengl commented Aug 13, 2018
^ Accidental tag removal and addition. Sorry! |
Fixes: nodejs/node#13523 PR-URL: nodejs/node#22261 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jon Moss <[email protected]>
Fixes: #13523
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes