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

lib: update punycode to 2.3.0 #46719

Closed
wants to merge 2 commits into from
Closed

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 force-pushed the lib-punycode-update branch from d31feb8 to 1f683af Compare February 18, 2023 17:00
@Trott
Copy link
Member

Trott 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?)

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

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

@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 force-pushed the lib-punycode-update branch from 9e9129e to 82b66f6 Compare February 18, 2023 18:21
@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

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:

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

@nodejs-github-bot
Copy link
Collaborator

@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
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 anonrig force-pushed the lib-punycode-update branch from 82b66f6 to 2a97606 Compare February 19, 2023 22:23
@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 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

@nodejs-github-bot
Copy link
Collaborator

@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
@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
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
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
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
PR-URL: #46719
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
@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
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
PR-URL: #46719
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

7 participants