punycode: fix jsdoc references

[anonrig]

No description provided.

[nodejs-github-bot]

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

[mscdex]

This file is actually a (deprecated) 3rd party dependency originating from here, so changes should be made there instead.

[Trott]

This file is actually a (deprecated) 3rd party dependency originating from here, so changes should be made there instead.

We have definitely changed it, though:

1. Adding a deprecation warning: https://github.com/nodejs/node/pull/38444/files
2. Using strict equality checks: https://github.com/nodejs/node/pull/30898/files

The most recent version (which we are using with those two modifications above) is 2.1.1, released more than 4 years ago. I think we decided at some point that we would maintain our own fork here, but maybe I'm mistaken about that.

[anonrig]

This file is actually a (deprecated) 3rd party dependency originating from here, so changes should be made there instead.

The referenced repository is not maintained anymore.

• Last commit October 23, 2020
• Last NPM release 4 years ago.

[mscdex]

Perhaps we could convince @mathiasbynens to upstream our existing changes (sans node-specific stuff like the deprecation warning) + these doc fixes?

[mathiasbynens]

Perhaps we could convince @mathiasbynens to upstream our existing changes (sans node-specific stuff like the deprecation warning) + these doc fixes?

You totally could. Please send a PR :)

[anonrig]

If we are going to upstream the changes, should we add punycode as a dependency?

[Trott]

If we are going to upstream the changes, should we add punycode as a dependency?

It's already in the LICENSE file. Do you mean, though, that it should be moved to the deps directory? Semantically, that makes some sense, but I wonder if that would introduce more practical friction than benefit. (I'm not sure, but my guess is "yes".)

[anonrig]

It's already in the LICENSE file. Do you mean, though, that it should be moved to the deps directory? Semantically, that makes some sense, but I wonder if that would introduce more practical friction than benefit. (I'm not sure, but my guess is "yes".)

So, I opened a pull request for these changes and it got merged. What is the next step for that change to get into the node.js core? If it was deps it would be a github ci workflow, that automatically does this. But since this isn't a deps (semantically speaking), I'm in the unknown territories.

[Trott]

So, I opened a pull request for these changes and it got merged. What is the next step for that change to get into the node.js core?

I would recommend this approach:

• Make sure 9c460e1 is also upstreamed
  • fix: upstream node.js changes mathiasbynens/punycode.js#121
• Make sure that your changes and 9c460e1 are in the most recent release of punycode. If not, ask mathiasbynens to publish a release.
  • Release a new version on Github & NPM mathiasbynens/punycode.js#123
• Copy the released code into lib/punycode.js as one commit. Add a second commit by cherry-picking 40ea8aa onto that commit. Open a pull request with the two commits and set the commit-queue-rebase label.
  • lib: update punycode to 2.3.0 #46719
• Optional: If successful, add a GitHub action workflow to automate this.
• Optional: Find a way to upstream 40ea8aa. Maybe find a way to do a runtime check that you are running Node.js and only run process.emitWarning in that situation. Or maybe better yet, find a way to emit the warning within the code that loads built-in modules rather than in punycode.js itself. If we do it that way, we don't need to upstream anything.

[anonrig]

I opened another pull request for upstreaming our internal changes. @mathiasbynens I appreciate if you can take a look at it (mathiasbynens/punycode.js#121)

[silverwind]

I think it's time to remove the core punycode module as it's been deprecated since 7.0.0 and its existance is a source of confusion because afaik it's undefined behaviour whether import 'punycode' will get you the core module or the npm module.

Edit: It appears there is a non-ambigous module on npm, so that avoids the undefined behaviour.

[anonrig]

The original author is unresponsive to my upstream changes pull request. I recommend continuing this pull request. @Trott What are your thoughts on this?

I think it's time to remove the core punycode module as it's been deprecated since 7.0.0

@silverwind I agree with you. What are the steps needed to remove this module from Node, without breaking any existing apps?

[Trott]

The original author is unresponsive to my upstream changes pull request. I recommend continuing this pull request. @Trott What are your thoughts on this?

It looks like the PR has been landed upstream.

[silverwind]

What are the steps needed to remove this module from Node, without breaking any existing apps?

I'd say next step is probably runtime deprecation, e.g. output a process warning when the module is used.

[targos]

If you enable pending deprecation warnings, you'll see that the ecosystem impact will be huge.
For example whatwg-url depends on the module

[silverwind]

Internal use should probably not output a warning, only external one. Or even better, refactor the internal use away from the module.

[targos]

I'm not talking about internal use. The whatwg-url npm module depends on tr46, which until recently required our punycode module. This changed in jsdom/tr46@fef6e95 (semver-major).
When I work with NODE_PENDING_DEPRECATION=1, I see the warning all the time because of it.

[silverwind]

I see, so that is because of same-name internal module and npm module, and I think this ambiguity is the reason why we now have such a hard time deprecating it without major impact.

I suggested in mathiasbynens/punycode.js#122 that the authors should recommend the unambiguous punycode.js npm name as the hack with the slash no longer works in ESM. So I guess this is the first step and then node's deprecation warning should point to the punycode.js module as well.

[silverwind]

Also, I recommend opening a new issue for this topic and stop abusing this PR 😉

[anonrig]

I'm closing this issue in favor of deprecation and the follow up pull requests done.