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

punycode: runtime deprecation #35092

Closed
wants to merge 1 commit into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Sep 7, 2020

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

@richardlau richardlau added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 7, 2020
@Trott
Copy link
Member

Trott commented Sep 7, 2020

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

@aduh95
Copy link
Contributor Author

aduh95 commented Sep 8, 2020

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
Copy link
Member

* 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
Copy link
Member

Trott commented Sep 8, 2020

@nodejs/tsc

@Trott Trott added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Sep 15, 2020
@Trott
Copy link
Member

Trott commented Sep 15, 2020

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

@ChALkeR
Copy link
Member

ChALkeR commented Sep 24, 2020

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?

Copy link
Member

@ChALkeR ChALkeR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Trott removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Oct 1, 2020
@Trott
Copy link
Member

Trott commented Oct 1, 2020

@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.

@aduh95 aduh95 force-pushed the runtime-deprecate-ponycode branch from 779b158 to 0ae2274 Compare October 1, 2020 21:42
@aduh95 aduh95 requested a review from ChALkeR October 1, 2020 21:42
@aduh95 aduh95 force-pushed the runtime-deprecate-ponycode branch from 0ae2274 to 877d5a9 Compare October 1, 2020 23:13
@@ -1,5 +1,15 @@
'use strict';

const { getOptionValue } = require('internal/options');
if (getOptionValue('--pending-deprecation')){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member

@ChALkeR ChALkeR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

jasnell commented Dec 16, 2020

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

@jasnell jasnell closed this Dec 16, 2020
@aduh95 aduh95 deleted the runtime-deprecate-ponycode branch December 17, 2020 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants