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

Update node-fetch to v3 #2415

Closed
char0n opened this issue Jan 14, 2022 · 9 comments · Fixed by #3137
Closed

Update node-fetch to v3 #2415

char0n opened this issue Jan 14, 2022 · 9 comments · Fixed by #3137
Assignees
Labels
dependencies Pull requests that update a dependency file released type: enhancement version: 3.x

Comments

@char0n
Copy link
Member

char0n commented Jan 14, 2022

node-fetch@3 is out for some it. Unfortunately it's pure ESM so we cannot use it. We need to convert this library to ESM to use it (#2414).

@char0n char0n added dependencies Pull requests that update a dependency file type: enhancement version: 3.x labels Jan 14, 2022
@reinrl
Copy link

reinrl commented Aug 22, 2022

Any updates on plans here? Trying to use swagger-client in a corporate environment, I am currently receiving security vulnerability warnings triggered by the version of node-fetch being used (with a recommended resolution to use node-fetch 3.2.10...or stop using swagger-client)

@char0n
Copy link
Member Author

char0n commented Sep 10, 2023

@reinrl,

Sorry for late response. In your case the solution is simple. Use Node.js >= 18 and npm overrides that would look like this:

{
  "overrides": {
    "swagger-client": {
      "node-fetch": "^3"
    }
  }
}

npm will not even install node-fetch@2 and your code will use native Node.js >=18 fetch (undici implementation).

@char0n
Copy link
Member Author

char0n commented Sep 10, 2023

After some research I came into the following conclusion: If we want to maintain backward compatibility (which we do) we have following options

1. Use native Node.js >= 18 fetch implementation

Wait until Node.js >=12 <18 dies out and we'll claim that swagger-client requires Node.js >= 18. For older Node.js following polyfills can be used:

  • for Node.js >= 12.20.0 <15 only node-fetch@3 polyfill can be used
  • for Node.js >=14, undici or node-fetch@3 polyfills can be used

2. Use node-fetch@3 with import() function

Given our usage of fetch function, this is currently very possible as we only use fetch function within async functions only.

3. Use node-fetch-cjs@3

https://www.npmjs.com/package/node-fetch-cjs is a CommonJS compatible version of node-fetch@3.


Generally I want to avoid integrating node-fetch@3 just to be doing the same, some time later with native Node.js fetch (undici impl). Pragmatically it just make sense to drop support for Node.js 12, and claim that SwaggerClient now supports Node.js >= 14.0.0. Use undici in isomorphic way - which means that browser env will use native browser fetch impl, Node.js >14 <18 will use the undici package, and Node.js >=18 will use the native Node.js fetch implementation (which is undici under the hood). So the solution would be the variation of 1..

@jimmywarting any comments?

@char0n
Copy link
Member Author

char0n commented Sep 11, 2023

Here is PR with Node.js >= 14 support and integrating undici: #3137

@jimmywarting
Copy link
Contributor

I would now recommend using:

  1. Built in fetch if available (would req NodeJS v18)
  2. Fallback to using undici (would req NodeJS v16.8)
  3. then lastly using node-fetch@3 (would req NodeJS ^12.20.0 || ^14.13.1 || >=16.0.0)

i would not recommend using the forked cjs version, using async import() works just fine from cjs, undici do actually lazy loaded fetch right in the source code.

// looks kind of like this in undici
module.exports.fetch = async function fetch (resource) {
    fetchImpl ??= require('./lib/fetch').fetch
    return await fetchImpl(...arguments)
}

it's no different from our recommendation in node-fetch/node-fetch#1279 (comment)

const fetch = (...args) => import('node-fetch').then(({default: fetch}) => fetch(...args))

That way you can still use node-fetch@3 from a cjs project while still loading esm-only modules.

node-fetch-cjs is not in sync with the main source.


It would also however be nice to not having to download any fetch dependency at all. if i'm using NodeJS v18 or v20 then i just feel that it's a bit in vain to have to download any extra dependency. maybe some other recommendation could be to just let the user polyfill fetch with whatever implementation they prefer. or make it a optional peer dependency where you try to load fetch from either

const fetch = globalThis.fetch || await import('undici')
  .then(mod => mod.fetch)
  .catch(err => import('node-fetch'))
  .then(mod => mod.fetch)
  .catch(err => {
    throw new Error('fetch is missing, this require nodejs v18 or you have to install node-fetch, undici or polyfill it your self')
  })

@jimmywarting
Copy link
Contributor

i would also say that it might just be the time to also drop cjs and only provide a esm-only
i think dual package is a hazard

@char0n
Copy link
Member Author

char0n commented Sep 12, 2023

Hi @jimmywarting,

Thanks for input.

Built in fetch if available (would req NodeJS v18)
Fallback to using undici (would req NodeJS v16.8)
then lastly using node-fetch@3 (would req NodeJS ^12.20.0 || ^14.13.1 || >=16.0.0)

Yeah, that's what I ended up doing during this night. It's good that my line of thinking wasn't that off. The only issue is that I'm testing exclusively against undici npm package in tests (in future when minimal supported Node.js is >18, we'll test exclusively against native Node.js fetch (which is again undici impl))

i would not recommend using the forked cjs version, using async import() works just fine from cjs, undici do actually lazy loaded fetch right in the source code.

Tried (and will probably look into it again), but it blows up the Jest. It would also blow up any downstream project using swagger-client.

Following jest config needs to be introduced to make it work:

  transformIgnorePatterns: [
    '/node_modules/(?!(node-fetch|data-uri-to-buffer|fetch-blob|formdata-polyfill)/)',
  ],

For not I've settled with: https://www.npmjs.com/package/node-fetch-commonjs

It would also however be nice to not having to download any fetch dependency at all. if i'm using NodeJS v18 or v20 then i just feel that it's a bit in vain to have to download any extra dependency. maybe some other recommendation could be to just let the user polyfill fetch with whatever implementation they prefer. or make it a optional peer dependency where you try to load fetch from either

Well that would be ideal, but our first goal is backward compatibility and out of the box functionality. If minimum supported version is Node.js >18, we will be there automatically, but our minimum supported Node.js is currently 12.20.0.

One can also decide to choose it's own fetch implementation by doing the following before importing swagger-client:

globalThis.fetch = myFetchImpl;
globalThis.Headers = myHeadersImpl;
globalThis.Request = myRequestImpl;
globalThis.Response = myResponseImpl;

i would also say that it might just be the time to also drop cjs and only provide a esm-only I think dual package is a hazard

As said above, that would break backward compatibility. We cannot afford this now. What we'll be doing is to introduce backward compatible ESM and we'll keep it as long as possible (until one of the dep we use happens to become ESM and there will be no replacement).

Thanks again @jimmywarting

@char0n
Copy link
Member Author

char0n commented Sep 12, 2023

Closed by #3137

@char0n char0n closed this as completed Sep 12, 2023
@char0n char0n self-assigned this Sep 12, 2023
char0n added a commit that referenced this issue Sep 12, 2023
swagger-client requires Node.js >=12.20.0 and uses different fetch 
implementation depending on Node.js version:

>=12.20.0 <16.8 - node-fetch@3
>=16.8 <18 - undici
>=18 - native Node.js fetch

Closes #1220
Closes #2736
Closes #2415
Closes #2381
Closes #2187
Closes #2291
swagger-bot pushed a commit that referenced this issue Sep 12, 2023
# [3.21.0](v3.20.2...v3.21.0) (2023-09-12)

### Features

* replace node-fetch with undici ([#3137](#3137)) ([bc7ca17](bc7ca17)), closes [#1220](#1220) [#2736](#2736) [#2415](#2415) [#2381](#2381) [#2187](#2187) [#2291](#2291)
@swagger-bot
Copy link
Contributor

🎉 This issue has been resolved in version 3.21.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file released type: enhancement version: 3.x
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants