-
Notifications
You must be signed in to change notification settings - Fork 10.9k
fix(core): resolve DEP0040 punycode deprecation via patch-package #17692
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
fix(core): resolve DEP0040 punycode deprecation via patch-package #17692
Conversation
Summary of ChangesHello @ATHARVA262005, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a solution to a Node.js deprecation warning encountered in newer Node.js versions by strategically patching a dependency. It ensures the continued functionality of URL parsing while eliminating the warning, thereby improving compatibility and maintaining the integrity of internationalized domain name (IDN) logic. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This PR addresses a deprecation warning in Node.js 21+ caused by the punycode module within the whatwg-url dependency. It introduces patch-package to apply a patch that redirects the dependency to use the userland punycode package. The changes include adding punycode and patch-package as dependencies, modifying the package-lock.json and package.json files, and adding a patch file. The review focuses on the addition of patch-package and the correctness of the patch application.
| "clean": "node scripts/clean.js", | ||
| "pre-commit": "node scripts/pre-commit.js" | ||
| "pre-commit": "node scripts/pre-commit.js", | ||
| "postinstall": "patch-package" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding patch-package as a postinstall script is a good way to ensure that patches are applied after dependencies are installed. However, if the intention is to only apply this patch in development environments, it might be better to conditionally apply it based on the NODE_ENV environment variable to avoid unnecessary overhead in production environments.
If it should always be applied, then this is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The postinstall script must run in all environments (including production) because whatwg-url is a runtime dependency used by @google-cloud/storage. If we skip patching in production, the [DEP0040] warning will persist in our production logs, which is exactly what we are trying to fix.
patches/whatwg-url+5.0.0.patch
Outdated
| -const punycode = require("punycode"); | ||
| +const punycode = require("punycode/"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The patch changes the require statement to point to the userland punycode package. While this resolves the deprecation warning, it's crucial to ensure that the userland package provides the exact same API and behavior as the deprecated built-in module to avoid unexpected issues. It would be helpful to add a comment in the patch file itself explaining why the trailing slash is necessary and that the userland package is API-compatible.
-const punycode = require("punycode");
+const punycode = require("punycode/"); // Trailing slash ensures userland package usage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the comment as requested. Regarding compatibility: the punycode userland package (v2.3.1) is the official polyfill for the deprecated Node.js core module and maintains strict API compatibility for the methods used here (ucs2.decode and toASCII). The trailing slash is the standard Node.js mechanism to force loading from node_modules."
ace536a to
059fd43
Compare
|
Hi there! Thank you for your contribution to Gemini CLI. We really appreciate the time and effort you've put into this pull request. To keep our backlog manageable and ensure we're focusing on current priorities, we are closing pull requests that haven't seen maintainer activity for 30 days. Currently, the team is prioritizing work associated with 🔒 maintainer only or help wanted issues. If you believe this change is still critical, please feel free to comment with updated details. Otherwise, we encourage contributors to focus on open issues labeled as help wanted. Thank you for your understanding! |
|
Updated Details for Reopening: This change is critical as it resolves the primary source of [DEP0040] warnings impacting all users on Node.js 21-25. I have just finished: Rebasing the branch onto the latest nightly to ensure zero conflicts with the 5 most recent commits. Verification on Node v22.12.0 confirming the warning is eliminated via the whatwg-url patch. Guideline Audit: Verified strict compliance with CONTRIBUTING.md. @Adib234 – As I was assigned to this yesterday, I’ve completed the final "surgical" fix. Could you please reopen this PR for final review? |
Summary
This PR resolves the
[DEP0040] DeprecationWarningcaused by thepunycodemodule in Node.js 21+. It redirects thewhatwg-urldependency to use the userlandpunycodepackage via a surgical patch, ensuring compatibility without breaking IDN logic.Details
The fix involves:
punycode(userland) as a dependency.whatwg-urlusingpatch-packageto replace the built-inrequire("punycode")withrequire("punycode/")(trailing slash ensures userland package usage).Related Issues
Closes #12289
How to Validate
node --trace-deprecationto confirm the warning often seen withwhatwg-urlis gone.Pre-Merge Checklist