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

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

Closed
wants to merge 3 commits into from

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Jul 21, 2023

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
Copy link
Collaborator

Review requested:

  • @nodejs/url

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. labels Jul 21, 2023
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 21, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 21, 2023
@nodejs-github-bot
Copy link
Collaborator

@lpinca
Copy link
Member

lpinca commented Jul 21, 2023

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
Copy link
Member Author

anonrig commented Jul 21, 2023

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

@aduh95 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
@lpinca
Copy link
Member

lpinca commented Jul 21, 2023

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

@anonrig
Copy link
Member Author

anonrig commented Jul 21, 2023

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
Copy link
Member

lpinca commented Jul 21, 2023

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
Copy link
Contributor

aduh95 commented Jul 21, 2023

@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
Copy link
Member Author

anonrig commented Jul 21, 2023

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

nodejs-github-bot pushed a commit that referenced this pull request Jul 22, 2023
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>
nodejs-github-bot pushed a commit that referenced this pull request Jul 22, 2023
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>
anonrig and others added 3 commits July 21, 2023 21:34
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>
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
Copy link
Member Author

anonrig commented Jul 22, 2023

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

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 22, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 22, 2023
@nodejs-github-bot
Copy link
Collaborator

@aduh95
Copy link
Contributor

aduh95 commented Jul 22, 2023

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

Ceres6 pushed a commit to Ceres6/node that referenced this pull request Jul 27, 2023
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>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Jul 27, 2023
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>
pluris pushed a commit to pluris/node that referenced this pull request Aug 6, 2023
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>
pluris pushed a commit to pluris/node that referenced this pull request Aug 6, 2023
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>
pluris pushed a commit to pluris/node that referenced this pull request Aug 7, 2023
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>
pluris pushed a commit to pluris/node that referenced this pull request Aug 7, 2023
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>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
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>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
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>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
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>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
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>
UlisesGascon pushed a commit to UlisesGascon/node that referenced this pull request Aug 14, 2023
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>
UlisesGascon pushed a commit to UlisesGascon/node that referenced this pull request Aug 14, 2023
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>
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
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>
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
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>
ruyadorno pushed a commit that referenced this pull request Aug 16, 2023
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>
ruyadorno pushed a commit that referenced this pull request Aug 16, 2023
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 pushed a commit that referenced this pull request Aug 16, 2023
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
Copy link
Member

Landed in f4617a4...69aaf8b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants