GeezerVison

anonrig requested review from aduh95 and richardlau

July 21, 2023 15:26

anonrig mentioned this pull request

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

Closed

lpinca approved these changes

View reviewed changes

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

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

nodejs-github-bot commented Jul 21, 2023

Copy link

Copy Markdown

Collaborator

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lpinca commented Jul 21, 2023 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

anonrig commented Jul 21, 2023

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

lpinca commented Jul 21, 2023

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

anonrig commented Jul 21, 2023

Copy link

Copy Markdown

Member Author

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().

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lpinca commented Jul 21, 2023

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

aduh95 commented Jul 21, 2023

Copy link

Copy Markdown

Contributor

@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.

lpinca and anonrig reacted with thumbs up emoji

All reactions

👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

anonrig commented Jul 21, 2023

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

benjamingr approved these changes

View reviewed changes

anonrig mentioned this pull request

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

Closed

nodejs-github-bot pushed a commit that referenced this pull request


          test: validate host with commas on url.parse

f4863fa

PR-URL: #48878
Refs: #48873
Refs: #48855
Refs: #48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>

nodejs-github-bot pushed a commit that referenced this pull request


          lib: remove invalid parameter to toASCII

9dd574c

PR-URL: #48878
Refs: #48873
Refs: #48855
Refs: #48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>

anonrig and others added 3 commits

July 21, 2023 21:34


          src: replace idna functions with ada::idna

6c602b3

Co-authored-by: Daniel Lemire <[email protected]>
PR-URL: nodejs#47735
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>


          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 <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>


          lib: remove invalid parameter to toASCII

60f1bb5

PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>

anonrig force-pushed the backport-47735 branch from 17649f7 to 60f1bb5 Compare

July 22, 2023 01:35

anonrig commented Jul 22, 2023

Copy link

Copy Markdown

Member Author

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

lpinca reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

anonrig added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label

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

nodejs-github-bot commented Jul 22, 2023

Copy link

Copy Markdown

Collaborator

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

aduh95 commented Jul 22, 2023

Copy link

Copy Markdown

Contributor

@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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Ceres6 pushed a commit to Ceres6/node that referenced this pull request

Jul 27, 2023


          test: validate host with commas on url.parse

932d92e

PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>

Ceres6 pushed a commit to Ceres6/node that referenced this pull request

Jul 27, 2023


          lib: remove invalid parameter to toASCII

9923dd9

PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>

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

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

Merged

Ceres6 pushed a commit to Ceres6/node that referenced this pull request


          test: validate host with commas on url.parse

ba96a46

PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>

Ceres6 pushed a commit to Ceres6/node that referenced this pull request


          lib: remove invalid parameter to toASCII

c433de3

PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>

Ceres6 pushed a commit to Ceres6/node that referenced this pull request


          test: validate host with commas on url.parse

6c0c565

PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>

Ceres6 pushed a commit to Ceres6/node that referenced this pull request


          lib: remove invalid parameter to toASCII

bb7ffa8

PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>

UlisesGascon pushed a commit to UlisesGascon/node that referenced this pull request


          test: validate host with commas on url.parse

17a1be6

PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>

UlisesGascon pushed a commit to UlisesGascon/node that referenced this pull request


          lib: remove invalid parameter to toASCII

5affce3

PR-URL: nodejs#48878
Refs: nodejs#48873
Refs: nodejs#48855
Refs: nodejs#48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>

RafaelGSS pushed a commit that referenced this pull request

Aug 15, 2023


          test: validate host with commas on url.parse

3680a66

PR-URL: #48878
Refs: #48873
Refs: #48855
Refs: #48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>

RafaelGSS pushed a commit that referenced this pull request

Aug 15, 2023


          lib: remove invalid parameter to toASCII

420b85c

PR-URL: #48878
Refs: #48873
Refs: #48855
Refs: #48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>

ruyadorno pushed a commit that referenced this pull request


          src: replace idna functions with ada::idna

e426180

Co-authored-by: Daniel Lemire <[email protected]>
PR-URL: #47735
Backport-PR-URL: #48873
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>

ruyadorno pushed a commit that referenced this pull request


          test: validate host with commas on url.parse

2de9868

PR-URL: #48878
Backport-PR-URL: #48873
Refs: #48873
Refs: #48855
Refs: #48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>

ruyadorno pushed a commit that referenced this pull request


          lib: remove invalid parameter to toASCII

69aaf8b

PR-URL: #48878
Backport-PR-URL: #48873
Refs: #48873
Refs: #48855
Refs: #48850
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>

ruyadorno commented Aug 16, 2023

Copy link

Copy Markdown

Member

Landed in f4617a4...69aaf8b

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ruyadorno closed this