lquixada / cross-fetch

Universal WHATWG Fetch API for Node, Browsers and React Native.
MIT License
1.67k stars 104 forks source link

node-fetch v2.7.0: punycode is deprecated in 'whatwg-url' and 'tr46' #177

Open spetrac opened 9 months ago

spetrac commented 9 months ago

Currently there are several issues raised for node-fetch v2.x (#1793, #1794, #1797) regarding the deprecation of punycode in whatwg-url up until v12.x (#261). There are over 5000 dependents on this module and using any of those packages outputs a deprecation warning regarding punycode.

https://github.com/lquixada/cross-fetch/blob/1fb2350e9e26bdc1a7903b8d5d051828503fbff6/src/node-ponyfill.js#L1-L2

https://github.com/lquixada/cross-fetch/blob/1fb2350e9e26bdc1a7903b8d5d051828503fbff6/src/node-ponyfill.js#L17-L19

As a solution I would suggest requiring the node-fetch module only when there is not already a fetch in the global scope (which was already introduced in node v17.5.0 and is stable since v21.0.0).

const nodeFetch = global.fetch || require('node-fetch') 
const realFetch = nodeFetch.default || nodeFetch 
exports.Headers = nodeFetch.Headers || global.Headers
exports.Request = nodeFetch.Request || global.Request
exports.Response = nodeFetch.Response || global.Response

With this everything should be backwards compatible and also upwards compatible with newer node versions. The only thing that might not be included is the agent options for the fetch method to inject a node http agent, but this feature of node-fetch is not a fetch standard anyways.

iegik commented 3 months ago

This because whatwg-fetch replaces global.fetch!! https://github.com/lquixada/cross-fetch/issues/184

pfoerdie commented 3 months ago

I don't think this is relevant here, because the rollup.config.js script, which uses whatwg-fetch, is meant to generate the browser version of the polyfill/ponyfill and is only used in the build step.

https://github.com/lquixada/cross-fetch/blob/1fb2350e9e26bdc1a7903b8d5d051828503fbff6/rollup.config.js#L1

This issue is not about the browser version, but about warning showing in the nodejs environment. And I think the problem and its solution is already described pretty accurate. It seems that the author of this package does not maintain it any further, because the last update is 1 year old and none of the issues was addressed ever since. I would advice library authors to find alternatives to this package for that reason.

pfoerdie commented 3 months ago

BTW, a quickfix for this issue is to create an override in the package.json for whatwg-url:

  "overrides": {
    "node-fetch@2.x": {
      "whatwg-url": "14.x"
    }
  },
mo commented 2 weeks ago

node 22 is soon LTS it would be nice to upgrade node-fetch to 3.x to get rid of the punycode warning

mo commented 2 weeks ago

running with NODE_OPTIONS="--disable-warning DEP0040" will disable the deprecation warning for punycode

spetrac commented 2 weeks ago

@mo I think disabling warnings is never a good solution, because those warning are not irrelevant, when any node version could finally deprecate the punycode module. There are more practical solutions, like overriding the whatwg-url dependency.

Regardless of the author not actively maintaining this package any more and ignoring any pull requests, upgrading this to node-fetch 3.x will also result in loosing support for older node versions. That means, the cross-fetch module would have to increase its version to 5.0.0, because this is a major change. This also means any depending libraries would have to update their dependencies manually which they could have done a long time ago to move away from this module. In modern environments it is not really needed anyways, because fetch is already adopted and dependencies like node-fetch 3.x or undici-fetch, which is nodes internal fetch library, could be used directly without the need for cross-fetch. If you know your environment, just pick the best fetch solution, and if you do not know your environment, using cross-fetch will not reduce your bundle-size if you do not need it, so just pick any fitting fetch-library directly. This approach will result in much more stable runtime behavior, because the fetch libraries are somewhat different actually, like http-Agent support in node-fetch or undicis dispatcher alternative. If you really don't want to override the global fetch if it already exists, just do it by yourself with global.fetch ??= require('node-fetch') or a similar way.

mo commented 2 weeks ago

@spetrac I have written a CLI tool and now every time it's used it prints a punycode warning which is really annoying. I get the warning because of this dependency chain (also also several other dependency chains like for example I use "jsdom" and "sentry" which both have transitive deps on old whatwg-url versions): ├─┬ puppeteer@21.10.0 │ └─┬ puppeteer-core@21.10.0 │ └─┬ cross-fetch@4.0.0 │ └─┬ node-fetch@2.7.0 │ └── whatwg-url@5.0.0 I've already upgraded all my direct dependencies and that doesn't fix the issue. I don't use cross-fetch or node-fetch myself, I only get them via transitive deps.

I tried adding an override in my top-level package.json like this: "overrides": { "whatwg-url": "14.x" }

...and while "npm ls whatwg-url" reports 14.0.0 for all instances of whatwg-url I still get the punycode error.

Suppressing warnings in general is of course bad and should not be done at all in libraries, but in my CLI application it's the only option to keep my app usable. Since I have no control of my transitive deps I cannot get rid of the warning in any other way (considering that the package.json "overrides" didn't work for me). And letting my end-users see the warning is of course even more pointless since they certainly cannot fix it, they are not even developers.

spetrac commented 2 weeks ago

I know a warning is pretty annoying for end-users, but it would be more annoying if the program crashes on their machine and they see no warning at all.

If you can confirm that no whatwg-url 5.x-12.x is used anywhere, I would suspect that this might not be the only module with the punycode issue. I know its tedious, but there is no help but dig deeper until all punycode issues are resolved.

BTW, in the current version of puppeteer 23.x they already dropped the cross-fetch dependency.

mo commented 2 weeks ago

@spetrac ah yes, you're right... I enabled --trace-warnings now and ran with the whatwg-url overrides in package.json and then I found this depency chain: └─┬ jsdom@23.0.1 └─┬ tough-cookie@4.1.3 └── psl@1.8.0 ...where psl package uses punycode as well and they havn't released any version without punycode yet (there is a fix but no maintainer for the package). It seems tough-cookie released 5.0 where they dropped "psl" as a dependency though. With these overrides I get no deprecation warning: "overrides": { "whatwg-url": "14.x", "tough-cookie": "5.x" }