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: TupleOrigin#toString use unicode by default #10552

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Dec 31, 2016

See: https://url.spec.whatwg.org/#dom-url-origin

Also moves the tests for origins to the parsing tests
since now URL#origin matches the test cases by default.

Aside: maybe the unicode argument can be removed since from what I've found so far the spec doesn't use domain to ASCII serialization for origins and https://coverage.nodejs.org/coverage-abc1633de649bfa5/root/internal/url.js.html shows that the unicode === false is never taken in tests.

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

cc / @jasnell

See: https://url.spec.whatwg.org/#dom-url-origin

Also moves the tests for origins to the parsing tests
since now URL#origin matches the test cases by default.
@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x url Issues and PRs related to the legacy built-in url module. labels Dec 31, 2016
@joyeecheung
Copy link
Member Author

Ping, is there anything else that needs to be addressed?

@addaleax
Copy link
Member

addaleax commented Jan 3, 2017

I don’t think this got a CI run so far, so: https://ci.nodejs.org/job/node-test-commit/7000/

jasnell pushed a commit to jasnell/node that referenced this pull request Jan 3, 2017
See: https://url.spec.whatwg.org/#dom-url-origin

Also moves the tests for origins to the parsing tests
since now URL#origin matches the test cases by default.

PR-URL: nodejs#10552
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Jan 3, 2017

Landed in 2177a38. Thank you!

@joyeecheung joyeecheung closed this Jan 4, 2017
@jasnell
Copy link
Member

jasnell commented Jan 4, 2017

oops! Thanks for closing @joyeecheung! Must have hit the wrong button when I added the landing comment :-)

@joyeecheung joyeecheung added the whatwg-url Issues and PRs related to the WHATWG URL implementation. label Jan 5, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
See: https://url.spec.whatwg.org/#dom-url-origin

Also moves the tests for origins to the parsing tests
since now URL#origin matches the test cases by default.

PR-URL: nodejs#10552
Reviewed-By: James M Snell <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 19, 2017
See: https://url.spec.whatwg.org/#dom-url-origin

Also moves the tests for origins to the parsing tests
since now URL#origin matches the test cases by default.

PR-URL: nodejs#10552
Reviewed-By: James M Snell <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 24, 2017
See: https://url.spec.whatwg.org/#dom-url-origin

Also moves the tests for origins to the parsing tests
since now URL#origin matches the test cases by default.

PR-URL: nodejs#10552
Reviewed-By: James M Snell <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
See: https://url.spec.whatwg.org/#dom-url-origin

Also moves the tests for origins to the parsing tests
since now URL#origin matches the test cases by default.

PR-URL: nodejs#10552
Reviewed-By: James M Snell <[email protected]>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
@joyeecheung joyeecheung deleted the whatwg-url-tostring branch February 19, 2017 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
url Issues and PRs related to the legacy built-in url module. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants