-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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(node): export punycode module #19151
Conversation
NodeModulePolyfill { | ||
name: "punycode", | ||
specifier: "ext:deno_node/punycode.ts", | ||
}, |
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 version of the punycode module bundled in Node.js is being deprecated. In a future major version of Node.js this module will be removed. Users currently depending on the punycode module should switch to using the userland-provided Punycode.js module instead.
Should we bother adding this if it's going to be deprecated?
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 think so, currently existing packages might still depend on it and it doesn't say when it will be removed. @kt3k thoughts?
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.
(and we already support it in CJS just not ESM)
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'm fine with either way, but probably it's less confusing if we already provide it via cjs. I confirmed the below prints the punycode module without error:
import { createRequire } from "node:module";
console.log(createRequire(import.meta.url)("node: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.
LGTM
Closes #19147