punycode: limit deprecation warning

[cjihrig]

DEP0040 is an extremely annoying warning. Most of the people seeing it cannot do anything about it. This commit updates the warning logic to only emit outside of node_modules. This is similar to other warnings such as the Buffer() constructor warning.

Ideally, this should be backported to Node 22.

Refs: #47202

[ljharb]

omg thank you

[anonrig]

I think this is 1 step backward toward removing punycode. I don't think we should do it.

[cjihrig]

I disagree. @targos pointed out in the original PR how much ecosystem impact this would have and it was disregarded. I would also like punycode removed, but this is currently only frustrating users who can't do anything about it... similar to the Buffer constructor.

I have tried working around this in other ways, and there is not a great solution IMO.

[anonrig]

I disagree. @targos pointed out in the original PR how much ecosystem impact this would have and it was disregarded. I would also like punycode removed, but this is currently only frustrating users who can't do anything about it... similar to the Buffer constructor.

I have tried working around this in other ways, and there is not a great solution IMO.

Why don't you add punycode npm package as a dependency to override this behavior?

[cjihrig]

If I add an override to my package.json, that override is not honored by npm when the package is used as a dependency by someone else.

[anonrig]

If I add an override to my package.json, that override is not honored by npm when the package is used as a dependency by someone else.

So, how do you propose continuing this deprecation, if we make it node modules only right now? When should we revert this Pr? What should be the condition? Original Pr, landed 2 years ago.

[cjihrig]

As a project we don't have a good answer for that. That's why the Buffer constructor is still there 😄

In this example, [email protected] still gets a massive amount of downloads... significantly more than the 3.x.x release line. If the issue could be fixed in that dependency tree (my understanding is that they refuse to do so), it would solve the problem for a lot of people.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.19%. Comparing base (0e7ec5e) to head (c6433a9).
Report is 13 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56632      +/-   ##
==========================================
- Coverage   89.20%   89.19%   -0.01%     
==========================================
  Files         662      662              
  Lines      191890   191898       +8     
  Branches    36928    36930       +2     
==========================================
- Hits       171176   171173       -3     
- Misses      13554    13565      +11     
  Partials     7160     7160              

Files with missing lines	Coverage Δ
lib/punycode.js	94.72% <100.00%> (+0.05%)	⬆️

... and 25 files with indirect coverage changes

[anonrig]

As a project we don't have a good answer for that. That's why the Buffer constructor is still there 😄

In this example, [email protected] still gets a massive amount of downloads... significantly more than the 3.x.x release line. If the issue could be fixed in that dependency tree (my understanding is that they refuse to do so), it would solve the problem for a lot of people.

Not to annoy but if this annoys you or others this much, I think the warning is doing its job correctly. I still think we should keep it as it is but @nodejs/TSC wdyt?

[cjihrig]

I think the warning is doing its job correctly

It isn't though. It's annoying, yes. It is not currently actionable for me though.

EDIT: For additional context, I have a project that relies on @kubernetes/client-node, which generates part of it's code using OpenAPITools/openapi-generator, which generates code using [email protected], which uses [email protected]. whatwg-url is fixed in newer versions, but [email protected] is not going to update it because they claim backwards compatibility to Node v4. Node core's fetch is unstable on Node v18 and v20.

[cjihrig]

So, how do you propose continuing this deprecation

Original Pr, landed 2 years ago.

I think the warning is doing its job correctly

I don't really think we are any closer to moving this deprecation forward. The massive usage of [email protected] is prohibitive to us removing punycode, and also indicates that the warning is not moving people the way we'd like.

[ljharb]

(To be clear, the proper fix isn't with an override, it's with adding punycode as an npm dep and doing the import/require of punycode/ instead of punycode)

However, indeed deprecation warnings inside node_modules are simply never actionable, and they only serve to burden maintainers - I don't think they ever accelerate node's ability to remove anything.

[mcollina]

lgtm

[mcollina]

Node core's fetch is unstable on Node v18 and v20.

We can mark it as such in node v20. It has the same come of v22.

[cjihrig]

We can mark it as such in node v20.

I think that, in conjunction with the ongoing work to backport require(esm) and Node 18 going EOL soon, would be great.

[anonrig]

Since this got enough attention, I'm removing my block.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[joyeecheung]

I think this should be the default for most deprecation warnings that will hit the high impact packages TBH.

[joyeecheung]

Also looking at the original motivation #7552, I am not very convinced that the deprecation is really worth the confusion it introduces to the ecosystem. IMO it is better to mark it as legacy instead of trying to deprecate it at this point (not to be done in this PR) as it is clearly still widely used despite having that warning for two years. New code is unlikely to use it now that there are faster builtin alternatives and old code is just going to be old code.

[nodejs-github-bot]

Landed in 840f952

[jufemaiz]

Looking at this and I'm not sure why a deprecated package would continue to be relied upon by node core packages while at the same time muting the deprecation warnings.

Is the package deprecated or not? If it is, where will the changes be made for the url package (for example) to make use of the punycode algorithm, currently implemented in the punycode package. Ref: https://nodejs.org/api/url.html#new-urlinput-base

[ljharb]

@jufemaiz i think you misunderstand. the deprecation applies to the node core punycode module, and this PR merely hides the error when it's not actionable - namely, when it's being used by someone else's code instead of by your own.

[jufemaiz]

@jufemaiz i think you misunderstand. the deprecation applies to the node core punycode module, and this PR merely hides the error when it's not actionable - namely, when it's being used by someone else's code instead of by your own.

We are seeing this as a result of the url module. Which is also a node core module.

What I'm trying to understand is what the strategy is for node core modules that currently depend on punycode given that the module is deprecated. This hides that warning, so when punycode is deleted, what happens to the other node core modules currently depending on another deprecated node core module?

[ljharb]

Nope, that's https://npmjs.com/url, which is not owned by node, and v0.11.4 of that package addresses the issue - so you may just need to upgrade.

[jufemaiz]

🤦🏼 thanks for highlighting that! Argh. Our internal review I read didn't pick that up.

Lesson learnt: always double check.