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.

Sign up for GitHub

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

Jump to bottom

lib: update punycode to 2.3.0 #46719

Closed
wants to merge 2 commits into from
Closed

lib: update punycode to 2.3.0 #46719

wants to merge 2 commits into from

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Feb 18, 2023

Resolving 3rd item on fixing jsdoc references in punycode.

Referencing: #45524 (comment)

cc @Trott

@anonrig anonrig added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Feb 18, 2023
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Feb 18, 2023
@anonrig anonrig mentioned this pull request Feb 18, 2023
Closed
@Trott
Copy link
Member

Trott commented Feb 18, 2023 •
edited
Loading

Does the second commit also need to update deprecations.md to change the deprecation from documentation-only to runtime? (And if so, does that make this semver major?)

Other than sorting that out, this looks good to me. Thanks for the follow-through on this.

@anonrig
Copy link
Member Author

anonrig commented Feb 18, 2023

Does the second commit also need to update deprecations.md to change the deprecation from documentation-only to runtime? (And if so, does that make this semver major?)

It is already deprecated. I just cherry picked and added the pending-deprecation code to punycode.js due to the changes on the first commit. Nothing changes, the second commit fixes the removal of the warning emit in the first commit.

@Trott
Copy link
Member

Trott commented Feb 18, 2023 •
edited
Loading

Does the second commit also need to update deprecations.md to change the deprecation from documentation-only to runtime? (And if so, does that make this semver major?)

It is already deprecated. I just cherry picked and added the pending-deprecation code to punycode.js due to the changes on the first commit. Nothing changes, the second commit fixes the removal of the warning emit in the first commit.

Oh, right, pending deprecation, and we're just refloating the change after updating the library. Yeah, that's fine. Thanks. Sorry for my misunderstanding!

@anonrig anonrig added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Feb 18, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 18, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@aduh95
Copy link
Contributor

aduh95 commented Feb 18, 2023

Note that this should probably not be landed using the CQ, otherwise the commit message for 1f683af would have two PR-URL metadata which could break our automations. How do you usually do when we float patch, e.g. on top of V8?

@anonrig
Copy link
Member Author

anonrig commented Feb 18, 2023

Note that this should probably not be landed using the CQ, otherwise the commit message for 1f683af would have two PR-URL metadata which could break our automations. How do you usually do when we float patch, e.g. on top of V8?

Should I remove the PR-URL from the description of the commit?

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 18, 2023
@aduh95
Copy link
Contributor

aduh95 commented Feb 18, 2023 •
edited
Loading

If you look at how it's done in #46445, all the metadata is stripped from the commit-message. I suggest doing that here as well.

Regarding 9e9129e, we wouldn't want to land this as a separate commit:

node/doc/contributing/pull-requests.md

Lines 540 to 541 in 5af2021

and the tests, but is still one logical change. All tests should always pass
when each individual commit lands on one of the `nodejs/node` branches.

Instead, you can use git commit --fixup 097a2806023b5a8498f792eff97e35c04588e04b to add follow up fixup! commit(s) that the CQ can understand and squash into the correct commit without you needing to rebase.

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

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

@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Feb 19, 2023
Open
21 tasks
@tniessen tniessen added dependencies Pull requests that update a dependency file. punycode Issues and PRs related to the punycode module bundled in Node.js. labels Feb 19, 2023
tniessen
tniessen reviewed Feb 19, 2023
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

I don't have the necessary context here, but for what it's worth, if the third commit is necessary due to the first commit, then the first commit is not self-contained and the third commit should likely be squashed into the first instead.

@anonrig
Copy link
Member Author

anonrig commented Feb 19, 2023

@aduh95 @tniessen You're right. I updated the initial commit to include the test changes and force pushed to this branch. Appreciate reviewing it again. Thanks!

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 19, 2023
@github-actions github-actions bot mentioned this pull request Feb 20, 2023
Open
24 tasks
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 20, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@targos
Copy link
Member

targos commented Feb 20, 2023

If you look at how it's done in #46445, all the metadata is stripped from the commit-message. I suggest doing that here as well.

Not really. deps: silence irrelevant V8 warning is the only commit cherry-picked from main and still has metadata. But you're right, I never land V8 updates with the commit queue and usually only keep the old metadata in cherry-picks.

@anonrig anonrig added the review wanted PRs that need reviews. label Feb 20, 2023
@github-actions github-actions bot mentioned this pull request Feb 21, 2023
Open
26 tasks
@anonrig anonrig added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 21, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 21, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in d1b29b4...629a8fa

nodejs-github-bot pushed a commit that referenced this pull request Feb 21, 2023
@anonrig @nodejs-github-bot
lib: update punycode to 2.3.0
be4f012 
PR-URL: #46719
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
nodejs-github-bot pushed a commit that referenced this pull request Feb 21, 2023
@aduh95 @nodejs-github-bot
punycode: add pending deprecation
629a8fa 
PR-URL: #46719
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
targos pushed a commit that referenced this pull request Mar 13, 2023
@anonrig @targos
lib: update punycode to 2.3.0
1c6a92b 
PR-URL: #46719
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
targos pushed a commit that referenced this pull request Mar 13, 2023
@aduh95 @targos
punycode: add pending deprecation
d3d76c3 
PR-URL: #46719
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
@targos targos mentioned this pull request Mar 14, 2023
Merged
@anonrig anonrig deleted the lib-punycode-update branch March 15, 2023 18:17
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
@anonrig @danielleadams
lib: update punycode to 2.3.0
ad510d9 
PR-URL: #46719
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
@aduh95 @danielleadams
punycode: add pending deprecation
fe318dd 
PR-URL: #46719
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
@danielleadams danielleadams mentioned this pull request Apr 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShogunPanda ShogunPanda ShogunPanda approved these changes

@Trott Trott Trott approved these changes

@tniessen tniessen Awaiting requested review from tniessen

@aduh95 aduh95 Awaiting requested review from aduh95

@targos targos Awaiting requested review from targos

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run. punycode Issues and PRs related to the punycode module bundled in Node.js. review wanted PRs that need reviews.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@anonrig @Trott @nodejs-github-bot @aduh95 @targos @ShogunPanda @tniessen