node-fetch
node-fetch copied to clipboard
node-fetch
node-fetch should provide a polyfill entrypoint
This has been an issue that has came back to me several times, but the fact that v3 is ESM-only module makes this a pain point with node-fetch. Read more bellow:
When is this an issue?
- using a library that expects fetch available as a global
- working with an isomorphic codebase
- you want to run a module/some code originally designed for the browser in your node codebase
Why are the existing solutions inappropriate?
The doc suggests doing this:
import fetch from 'node-fetch';
if (!globalThis.fetch) {
globalThis.fetch = fetch;
}
But this is actually problematic with ESM modules. If you use a library expecting fetch as a global (eg. react-relay-network-modern). There are typically three locations where you would import node-fetch:
- right before you import the library (unless you also run that code in the browser)
- in your server entry point
- in your bundler entry point
In all three cases using ES imports the above code won't be guaranteed to have assigned fetch on the global object before something is done with it, and so the only way to address that is to use an import file specifically for polyfilling.
Solutions
The correct solution is to provide a polyfill entrypoint that can be used for this purpose:
import 'node-fetch/polyfill';
This can either be implemented on this project or with an external package. An external package would either include node-fetch as a peer dependency, requiring the user to use 2 packages, or worse, it may be using node-fetch as a dependency.
Either way, this is an issue that will keep coming back until a proper solution is documented. Perhaps a middle-ground solution is to link to a module that properly polyfill node-fetch in the documentation.
Prior art and discussion
https://github.com/node-fetch/node-fetch/pull/119 (by yours truly) https://github.com/node-fetch/node-fetch/pull/521
Why does this belongs on this package?
-
This is a polyfill for
node, not for any other environment. I agree with @bitinn that providing abrowser.jsfile was out of scope:On https://github.com/node-fetch/node-fetch/pull/537
Thx for the PR but I believe this is better left for third-party module, where the author knows exactly what they want.
(we made mistake providing a
browser.jsand we regret it since, as people use it differently and sometimes in non-nodejs environment like react-native.) -
There is pretty much only one way to do it, https://github.com/node-fetch/node-fetch/pull/119 and https://github.com/node-fetch/node-fetch/pull/521 both use the same approach.
Why is it an even bigger issue today than 5 years ago
-
node-fetchv3 being an ESM-only module, it will often require creating a separate file to properly polyfill it - browser code is now fairly safe to use
fetchwithout any client polyfills and so it can be more frequently expected to rely on a global fetch
I'm on the fence. Would like hear someones else opinion. (mostly from the pioner at @node-fetch/core team, but also from everyone else)
I think we discussed this in the past in other issues?
With ESM and package.json we now have conditional exports, so we can export different code for different environments such as Node/Deno/Browsers/etc. As far as I know the tooling support for it is pretty good, and as it's now standardized in Node land, support for it will only grow.
I would suggest we move this into a separate package such as @node-fetch/universal or similar which only exports the fetch method where it's already available and node-fetch for Node
@gr2m this is unrelated to conditional exports though.
You would use conditional exports where you want an import present in multiple environments -- which on that subject, is arguably no a great approach IMO, as you may want to control how things are imported in userland, to e.g. rely on things like your browserlist.
Polyfilling as discussed here on the other hand is exclusively about exposing fetch in the global node environment. You only import it on a code path that you know is only used by node.
It can be a bit confusing because for example isomorphic-fetch does both of these. Perhaps polyfill is not the best term to use.
Got it, sorry. Nevermind my comment, we can hide it as off topic.
I don't have an opinion on your use case but I agree that it will come up. The node-fetch/polyfill approach sounds good to me, it's simple enough to add and maintain to keep it in the same package, but others might have thought more about it so let's get some more opinions on the matter
Can't it be done like this?
globalThis.fetch ||= (await import('node-fetch')).default
Can't it be done like this?
// lazy conditional import
if (!globalThis.fetch) {
const {default: fetch, Headers, Request, Response} = await import('node-fetch');
globalThis.fetch ||= fetch;
globalThis.Headers ||= Headers;
globalThis.Request ||= Request;
globalThis.Response ||= Response;
}
@jimmywarting By nesting it in an if statement, are we confident that the subclasses won't be missed out?
don't know, it's a bit unsafe... but it would also be weird if they where not b/c they go so hand in hand
In #1314 i wrote about exposing FormData, File and Blob as well... it's a bit annoying to install same dependencies also when node-fetch already has is and we could just as well use the same version to avoid dedup
@jimmywarting I may be wrong on this, but I think this may not need to go beyond fetch/Headers/Request/Response. In practice, if format objects or other things are used and need to be 'globaled' as well then they should probably be declared as separate dependencies?
I don't know of any lib that rely on fetch that also need those accessible globally, and if a path in your codebase does, then you would explicitly import, or polyfill those as required. if the same versions are used, the bundler will optimize bundle size. Besides, some of these things are making their way into node native. Perhaps this goes more into peerDependencies strategy land.
If it does come up as a real use case where we find it's actually needed, then maybe you have different level of polyfilling and you always use the lowest one needed? e.g.
import 'node-fetch/polyfill-level0' // => fetch
import 'node-fetch/polyfill-level1' // => fetch, Headers, Request, Response
import 'node-fetch/polyfill-level2' // => fetch, Headers, Request, Response, FormData, File, Blob
As for the import itself, I would make it static, the polyfill would typically be called just once, just above in the code tree before importing an isomorphic code path or the first lib that requires it, or in your build entry point. It's not meant to be used outside of node, so when you call it, nominally globalThis.fetch should not be defined. It could/should probably alert if the global was already defined. This follows the babel polyfill pattern. And if you are writing some node code that uses fetch, you should still use an explicit dependency and let the module system sort it out.
@goya so the feature that you linked (browser.js), is unrelated, it's a conditional export to use node-fetch in the browser. which was a bad idea to start with IMO. node-fetch as it names indicates should only be used in the node environment.
On the other hand, offering an option to expose fetch on the global node environment to replicate the browser API should had been offered all along as an option vs. documented as code to copy-paste, as the method of doing so depend too much on node-fetch implementation and has changed with different releases.
This is a fairly simple feature, but I think the confusion around it shows how important it is to get that stuff right, especially for downstream packages that depend on fetch to not need to assume anything about their environment.
yes, we used node-fetch in our sdk as it allowed our sdk users to package it for the browser or nodejs.
but we're happy to stick to < 3
You probably shouldn't do that though, your bundler/packaging script should allow you to handle this with entry points. This will avoid leaking abstractions, reduce your bundle size a bit, and simplify things when you have forks in how you handle environment initialization.
Any updates on this?
wondering that as well, a bit on the fence here... if it ain't necessary then maybe we can leave it as is... it's harder to revert something that got released out to the open...
"If it ain't broke, don't fix it"
May I ask what is the reason of not providing CommonJS lib? Ran into defect with utf8 urls and changing packaging system for workaround does not seem to be the best or viable option.
