Skip to content

url, i18n: update IDNA handling#13362

Closed
TimothyGu wants to merge 1 commit intonodejs:masterfrom
TimothyGu:url-idna
Closed

url, i18n: update IDNA handling#13362
TimothyGu wants to merge 1 commit intonodejs:masterfrom
TimothyGu:url-idna

Conversation

@TimothyGu
Copy link
Member

@TimothyGu TimothyGu commented Jun 1, 2017

The PR is still somewhat of a WIP, as a proof-of-concept that the proposed changes to the URL Standard work (whatwg/url#309).

Refs: whatwg/url#309
Refs: web-platform-tests/wpt#5976

/cc @domenic @zimbabao

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

@TimothyGu TimothyGu requested review from jasnell, srl295 and watilde June 1, 2017 06:38
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. i18n-api Issues and PRs related to the i18n implementation. labels Jun 1, 2017
@TimothyGu TimothyGu mentioned this pull request Jun 1, 2017
Merged
refack
refack reviewed Jun 1, 2017
View reviewed changes
src/node_i18n.cc Outdated
Copy link
Contributor

@refack refack Jun 1, 2017

Choose a reason for hiding this comment

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

nit: align white space in comment?

Copy link
Member Author

@TimothyGu TimothyGu Jun 3, 2017

Choose a reason for hiding this comment

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

It was intended to be a continuation of the previous line due to the line length lint rule, thus indented.

Copy link
Contributor

@refack refack Jun 3, 2017

Choose a reason for hiding this comment

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

Ack

refack
refack approved these changes Jun 1, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

LGTM % nit and a non-blocking request

src/node_i18n.cc Outdated
Copy link
Contributor

@refack refack Jun 1, 2017

Choose a reason for hiding this comment

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

👍 love the comment.

src/node_i18n.cc Outdated
Copy link
Contributor

@refack refack Jun 1, 2017

Choose a reason for hiding this comment

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

[question] AFAICT this is the only call to the workhorse ToASCII, so why implement IDNA_STRICT? Future proofing?

Copy link
Contributor

@refack refack Jun 1, 2017

Choose a reason for hiding this comment

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

So since it's not exposed maybe add a test case for IDNA_STRICT in test/cctest/test_url.cc

Copy link
Member Author

@TimothyGu TimothyGu Jun 3, 2017

Choose a reason for hiding this comment

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

The strict mode corresponds to beStrict variable in the algorithm. I'll make a mention of this in the code.

Copy link
Contributor

@refack refack Jun 3, 2017

Choose a reason for hiding this comment

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

[follow up]so in the current implementation IDNA_STRICT exists only in the sense that !IDNA_STRICT <=> IDNA_DEFAULT || IDNA_LENIENT?

@zimbabao
Copy link
Contributor

zimbabao commented Jun 1, 2017

LGTM

watilde
watilde approved these changes Jun 1, 2017
View reviewed changes
Copy link
Member

@watilde watilde left a comment

Choose a reason for hiding this comment

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

LGTM so far. Let's wait till upstream gets merge the PRs :)

@TimothyGu
Copy link
Member Author

TimothyGu commented Jun 3, 2017

Rebased, @refack's comment addressed. The upstream PRs were landed so this should be good to go.

CI: https://ci.nodejs.org/job/node-test-pull-request/8450/

@TimothyGu
Copy link
Member Author

TimothyGu commented Jun 4, 2017 •
edited
Loading

Redundant test cases removed. One last CI: https://ci.nodejs.org/job/node-test-pull-request/8466/ https://ci.nodejs.org/job/node-test-pull-request/8525/

@TimothyGu
url: update IDNA handling
48dc28e 
Remove custom tests for invalid IDNA domains in url-idna.js in favor of
the more comprehensive official set.

Refs: whatwg/url#309
Refs: web-platform-tests/wpt#5976
@TimothyGu
Copy link
Member Author

TimothyGu commented Jun 7, 2017

Landed in 91a1bbe.

@TimothyGu TimothyGu closed this Jun 7, 2017
@TimothyGu TimothyGu deleted the url-idna branch June 7, 2017 06:36
TimothyGu added a commit to TimothyGu/node that referenced this pull request Jun 7, 2017
@TimothyGu
url: update IDNA handling
91a1bbe 
Remove custom tests for invalid IDNA domains in url-idna.js in favor of
the more comprehensive official set.

PR-URL: nodejs#13362
Refs: whatwg/url#309
Refs: web-platform-tests/wpt#5976
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
jasnell pushed a commit that referenced this pull request Jun 7, 2017
@TimothyGu @jasnell
url: update IDNA handling
ba817d3 
Remove custom tests for invalid IDNA domains in url-idna.js in favor of
the more comprehensive official set.

PR-URL: #13362
Refs: whatwg/url#309
Refs: web-platform-tests/wpt#5976
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
@MylesBorins
Copy link
Contributor

MylesBorins commented Jul 17, 2017

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace do-not-land if it is being backported

@TimothyGu
Copy link
Member Author

TimothyGu commented Jul 18, 2017

dont-land is okay for this one, as the WHATWG URL parser is not in v6.x.

TimothyGu added a commit to TimothyGu/node that referenced this pull request Nov 28, 2017
@TimothyGu
url: update IDNA handling
79147ae 
Remove custom tests for invalid IDNA domains in url-idna.js in favor of
the more comprehensive official set.

PR-URL: nodejs#13362
Refs: whatwg/url#309
Refs: web-platform-tests/wpt#5976
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
@TimothyGu TimothyGu mentioned this pull request Nov 28, 2017
Closed
4 tasks
MylesBorins pushed a commit that referenced this pull request Jan 18, 2018
@TimothyGu @MylesBorins
url: update IDNA handling
5a13d1a 
Remove custom tests for invalid IDNA domains in url-idna.js in favor of
the more comprehensive official set.

Backport-PR-URL: #17365
PR-URL: #13362
Refs: whatwg/url#309
Refs: web-platform-tests/wpt#5976
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Feb 11, 2018
@TimothyGu @MylesBorins
url: update IDNA handling
4d73c47 
Remove custom tests for invalid IDNA domains in url-idna.js in favor of
the more comprehensive official set.

Backport-PR-URL: #17365
PR-URL: #13362
Refs: whatwg/url#309
Refs: web-platform-tests/wpt#5976
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 12, 2018
@TimothyGu @MylesBorins
url: update IDNA handling
dc1607b 
Remove custom tests for invalid IDNA domains in url-idna.js in favor of
the more comprehensive official set.

Backport-PR-URL: #17365
PR-URL: #13362
Refs: whatwg/url#309
Refs: web-platform-tests/wpt#5976
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 13, 2018
@TimothyGu @MylesBorins
url: update IDNA handling
60b10f0 
Remove custom tests for invalid IDNA domains in url-idna.js in favor of
the more comprehensive official set.

Backport-PR-URL: #17365
PR-URL: #13362
Refs: whatwg/url#309
Refs: web-platform-tests/wpt#5976
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
@aliscco aliscco mentioned this pull request Apr 27, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@watilde watilde watilde approved these changes

@srl295 srl295 Awaiting requested review from srl295

+1 more reviewer

@refack refack refack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. i18n-api Issues and PRs related to the i18n implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@TimothyGu @zimbabao @MylesBorins @refack @jasnell @watilde @nodejs-github-bot