punycode: runtime deprecation

[aduh95]

punycode is doc deprecated since v7 and the npm package is downloaded 40M per week, I think it's time to move its deprecation forward.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[Trott]

Might we want to leave it as a doc deprecation indefinitely?

[aduh95]

Might we want to leave it as a doc deprecation indefinitely?

Here are a few arguments why we might want to take it out of core eventually:

• That's 12kiB of JS that doesn't get used on core (except when ICU is disabled, see idna: use url module instead of punycode #35091).
• Removing it would not break any library, users would "just" have to do an additional npm i punycode if it wasn't listed on the library dependencies already.
• The npm stats seem to indicate that the Node.js community do not rely on the core module, so a runtime deprecation may have close to zero effect on the ecosystem. This hypothesis can be tested by a CITGM run.

[richardlau]

* The npm stats seem to indicate that the Node.js community do not rely on the core module, so a runtime deprecation may have close to zero effect on the ecosystem. This hypothesis can be tested by a CITGM run. 

Caution here. CITGM can alert if there may be a problem (e.g. a module fails), but a passing* CITGM run does not mean there is no effect in the ecosystem, merely that there was no observable effect to the modules that are tested in CITGM.

(* it's rare but has happened in the past)

[Trott]

@nodejs/tsc

[Trott]

TSC should probably decide to close this or move forward with it or else it will stall. Adding tsc-agenda label.

[ChALkeR]

This does not appear to be correct:

The npm stats seem to indicate that the Node.js community do not rely on the core module

Using an dataset from last year, top 3 punicode users are:

1. https://www.npmjs.com/package/tough-cookie — 27m/week, uses npm dep ✔️
2. https://www.npmjs.com/package/psl — 17m/week, does not use npm dep ❌
3. https://www.npmjs.com/package/whatwg-url — 16m/week, does not use npm dep ❌

I don't this the ecosystem is ready for this yet. Perhaps we could open issues first.

Or make this work with --pending-deprecation?

[ChALkeR]

I think this is likely too disruptive atm.

Perhaps this should be changed to support --pending-deprecation first, and then we could file issues/prs to top npm modules, pointing at that --pending-deprecation?

[Trott]

@aduh95 What do you think of @ChALkeR's suggestion above about using --pending-deprecation first? I would be happy to approve this PR if that were the change.

[ChALkeR]

Suggested change

	if(getOptionValue('--pending-deprecation')){
	if(getOptionValue('--pending-deprecation')){

Linter doesn't process this file as it's in .eslintignore.

[ChALkeR]

This is looking good in principle, but breaks when Node.js is configured with --without-intl flag:

$ ./node --pending-deprecation --trace-deprecation -e 'require("dns")'(node:858797) [DEP0040] DeprecationWarning: The `punycode` module is deprecated. Please use a userland alternative instead. at punycode.js:5:10 at NativeModule.compileForInternalLoader (internal/bootstrap/loaders.js:276:7) at nativeModuleRequire (internal/bootstrap/loaders.js:305:14) at internal/idna.js:7:34 at NativeModule.compileForInternalLoader (internal/bootstrap/loaders.js:276:7) at nativeModuleRequire (internal/bootstrap/loaders.js:305:14) at url.js:30:21 at NativeModule.compileForInternalLoader (internal/bootstrap/loaders.js:276:7) at nativeModuleRequire (internal/bootstrap/loaders.js:305:14) at internal/modules/cjs/helpers.js:19:17

To fix:

• Move the original lib/punycode.js to lib/internal/punycode.js
• Fix require('punycode') to require('internal/punycode') in lib/internal/idna.js.
• Introduce a wrapper in lib/punycode.js that does the pending deprecation logic and reexports require('internal/punycode'), like lib/sys.js does with require('util') (but with --pending-deprecation).
• Fix lib/punycode.js to lib/internal/punycode.js in .eslintignore

Should be good after that if I didn't miss any steps.

[jasnell]

Given the -1's (which I agree with), this is not something we're likely to do right now. Closing