[v18.x backport] src: replace idna functions with ada::idna #48873

anonrig · 2023-07-21T15:24:40Z

This PR includes a slightly modified backport which moves the function declarations to node_url.cc instead of encoding_binding.cc where Node.js v18 does not have. I've also added a new test to validate the correctness of this backport.

Backport #47735
Backport #48878

Fixes #48855
Fixes #48850

nodejs-github-bot · 2023-07-21T15:24:44Z

Review requested:

@nodejs/url

nodejs-github-bot · 2023-07-21T15:43:50Z

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

lpinca · 2023-07-21T15:52:05Z

test/parallel/test-url-parse.js does not exist in #47735. I would not create a new test file but update test/parallel/test-url-parse-format.js and also add it on main.

anonrig · 2023-07-21T15:54:00Z

@lpinca I was going to follow up and create test-url-parse.js in main as well.

lpinca · 2023-07-21T15:55:55Z

Ok but I don't think we need a new test file. Why not reusing test/parallel/test-url-parse-format.js?

anonrig · 2023-07-21T15:57:38Z

Ok but I don't think we need a new test file. Why not reusing test/parallel/test-url-parse-format.js?

My only argument is that the file name suggests url.format() where it only contains the calls to url.format().

lpinca · 2023-07-21T16:00:09Z

test-url-parse.js with a single special url in it does not make much sense to me. At least rename it to something like test-url-parse-hostname-with-comma.js

aduh95 · 2023-07-21T16:01:09Z

@lpinca I was going to follow up and create test-url-parse.js in main as well.

I think we should land it on main first, fast-track it, and backport it in this PR.

anonrig · 2023-07-21T16:02:51Z

test-url-parse.js with a single special url in it does not make much sense to me. At least rename it to something like test-url-parse-hostname-with-comma.js

Well, once you give a man a inferior choice, he'll appreciate the current choice he has, and goes ahead with test-url-parse-format.js. I'll wait for the tests to finish, to check if everything is ok, and later force push the file change. @lpinca

PR-URL: #48878 Refs: #48873 Refs: #48855 Refs: #48850 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

Co-authored-by: Daniel Lemire <daniel@lemire.me> PR-URL: nodejs#47735 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

PR-URL: nodejs#48878 Refs: nodejs#48873 Refs: nodejs#48855 Refs: nodejs#48850 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

anonrig · 2023-07-22T01:35:21Z

@lpinca @aduh95 Updated the backport to include the changes in main

nodejs-github-bot · 2023-07-22T01:55:51Z

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

aduh95 · 2023-07-22T08:10:55Z

@nodejs/lts it would be nice to have a patch release of v18.x with these commits to fix the regression on legacy url.parse.

PR-URL: nodejs#48878 Refs: nodejs#48873 Refs: nodejs#48855 Refs: nodejs#48850 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

PR-URL: #48878 Refs: #48873 Refs: #48855 Refs: #48850 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

Co-authored-by: Daniel Lemire <daniel@lemire.me> PR-URL: #47735 Backport-PR-URL: #48873 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

PR-URL: #48878 Backport-PR-URL: #48873 Refs: #48873 Refs: #48855 Refs: #48850 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

ruyadorno · 2023-08-16T00:42:01Z

Landed in f4617a4...69aaf8b

anonrig requested review from aduh95 and richardlau July 21, 2023 15:26

anonrig mentioned this pull request Jul 21, 2023

[v18.x] Revert "url: drop ICU requirement for parsing hostnames" #48869

Closed

lpinca approved these changes Jul 21, 2023

View reviewed changes

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 21, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 21, 2023

aduh95 changed the title ~~[v18.x] backport: src: replace idna functions with ada::idna~~ [v18.x backport] src: replace idna functions with ada::idna Jul 21, 2023

benjamingr approved these changes Jul 21, 2023

View reviewed changes

anonrig mentioned this pull request Jul 21, 2023

test: validate host with commas on url.parse #48878

Closed

anonrig and others added 3 commits July 21, 2023 21:34

src: replace idna functions with ada::idna

6c602b3

Co-authored-by: Daniel Lemire <daniel@lemire.me> PR-URL: nodejs#47735 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>

test: validate host with commas on url.parse

a0edb33

PR-URL: nodejs#48878 Refs: nodejs#48873 Refs: nodejs#48855 Refs: nodejs#48850 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

lib: remove invalid parameter to toASCII

60f1bb5

PR-URL: nodejs#48878 Refs: nodejs#48873 Refs: nodejs#48855 Refs: nodejs#48850 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

anonrig force-pushed the backport-47735 branch from 17649f7 to 60f1bb5 Compare July 22, 2023 01:35

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 22, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 22, 2023

anonrig mentioned this pull request Aug 2, 2023

yarn 1.x.x invalid url with node 18.17.0 VS 18.16.1 #48855

Closed

lpinca mentioned this pull request Aug 10, 2023

"URL.parse()" works differently in diff versions of Nodejs #49024

Closed

styfle mentioned this pull request Aug 14, 2023

Test with latest node@18 vercel/next.js#54000

Merged

ruyadorno closed this Aug 16, 2023

This was referenced Aug 28, 2023

Can't resolve names of containers in the same docker network if their names contain dots #49223

Closed

url.parse regression in 18.17+ #49330

Closed

URL.parse fails on URLs with comma separated hosts #48850

Closed

Uh oh!

[v18.x backport] src: replace idna functions with ada::idna #48873

[v18.x backport] src: replace idna functions with ada::idna #48873

Uh oh!

Conversation

anonrig commented Jul 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jul 21, 2023

Uh oh!

nodejs-github-bot commented Jul 21, 2023

Uh oh!

lpinca commented Jul 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anonrig commented Jul 21, 2023

Uh oh!

lpinca commented Jul 21, 2023

Uh oh!

anonrig commented Jul 21, 2023

Uh oh!

lpinca commented Jul 21, 2023

Uh oh!

aduh95 commented Jul 21, 2023

Uh oh!

anonrig commented Jul 21, 2023

Uh oh!

anonrig commented Jul 22, 2023

Uh oh!

nodejs-github-bot commented Jul 22, 2023

Uh oh!

aduh95 commented Jul 22, 2023

Uh oh!

ruyadorno commented Aug 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

anonrig commented Jul 21, 2023 •

edited

Loading

lpinca commented Jul 21, 2023 •

edited

Loading