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

url: expose WHATWG url.origin as ASCII #13126

Closed
wants to merge 1 commit into from

Conversation

TimothyGu
Copy link
Member

Refs: whatwg/url#297
Refs: web-platform-tests/wpt#5944

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

url

@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x whatwg-url Issues and PRs related to the WHATWG URL implementation. labels May 20, 2017
const unicodeHost = unicode ? domainToUnicode(host) : host;
return `${scheme}//${unicodeHost}${port === null ? '' : `:${port}`}`;
// Refs: https://html.spec.whatwg.org/multipage/browsers.html#ascii-serialisation-of-an-origin
function serializeTupleOrigin(scheme, host, port) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe keep https://html.spec.whatwg.org/multipage/browsers.html#unicode-serialisation-of-an-origin or quote the Note: There used to also be a Unicode serialization of an origin. However, it was never widely adopted.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a need to? This function is purely internal, and there is no remnant from the previous Unicode serialization that needs explanation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the standard kept a "there was something here", I thought we could emulate.
But I yield to your decision.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Lgtm. This part was always a bit sketchy to me since I knew browsers weren't consistent. Glad to see this fixed

@joyeecheung
Copy link
Member

@TimothyGu
Copy link
Member Author

Landed in 413691f.

@TimothyGu TimothyGu closed this May 25, 2017
@TimothyGu TimothyGu deleted the url-origin-ascii branch May 25, 2017 17:33
TimothyGu added a commit that referenced this pull request May 25, 2017
PR-URL: #13126
Refs: whatwg/url#297
Refs: web-platform-tests/wpt#5944
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
jasnell pushed a commit that referenced this pull request May 25, 2017
PR-URL: #13126
Refs: whatwg/url#297
Refs: web-platform-tests/wpt#5944
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
jasnell pushed a commit that referenced this pull request May 28, 2017
PR-URL: #13126
Refs: whatwg/url#297
Refs: web-platform-tests/wpt#5944
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@jasnell jasnell mentioned this pull request May 28, 2017
TimothyGu added a commit to TimothyGu/node that referenced this pull request Jul 25, 2017
Fixes: 413691f "url: expose WHATWG url.origin as ASCII"
Refs: nodejs#13126
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants