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

chore(cjs/esm): Bundle module and use package exports #1227

Closed
wants to merge 1 commit into from

Conversation

SomaticIT
Copy link
Contributor

THIS IS A PROPOSAL

As explained in #1226, the fact that node-fetch v3.0.0-beta.10 is now pure ESM cause multiple problems with existing codebases.

In JavaScript codebases, we need to use the async import() which can create a lot of changes.
In TypeScript codebases (with cjs module mode), it is impossible to use node-fetch anymore.

I think that ESM is the future but it is not as stable as we think and it is not ready to be used everywhere.
The best way to ensure that all existing codebases can migrate to v3 is to provide both cjs and esm versions of this module.

For this, I introduced esbuild which can transpile ESM to CJS and also can bundle the package.

You can say: "node does not need packages to be bundled", which is true.
However, bundling is a good practice to reduce the module loading time, disk operations, memory footprint, etc.
It can also be used to bundle internal dependencies and so reduce the npm dependency tree (reduce install time, disk operations, memory footprint, npm hell, etc.).

What is the purpose of this pull request?

  • Documentation update
  • Bug fix
  • New feature
  • Other, please explain:
    • allow both esm and cjs exports
    • reduce module loading time
    • reduce disk operations
    • reduce module memory footprint

What changes did you make? (provide an overview)

  • use esbuild to bundle module in esm and cjs format
  • bundle dependencies to reduce module size
  • use package exports to automatically select the appropriate version
  • add build step to prepublishOnly
  • add build step to ci

Which issue (if any) does this pull request address?

#1226
#1217

Is there anything you'd like reviewers to know?

In this implementation, I bundle both data-uri-to-buffer and fetch-blob into the final result allowing us to be dependency free.

I think data-uri-to-buffer is safe to bundle because it's an internal dependency.
However, I think fetch-blob should not be bundled because it could be used by the consumer.

What do you think?

Note 1: If we do not want to bundle fetch-blob, we have to provide a CJS export for it.
Note 2: If we do not bundle fetch-blob, the install size will be very large as mentioned by @akaupi in #1217. Maybe we could make fetch-blob dependency free by using esbuild to bundle web-streams-polyfill .

- use esbuild to bundle module in esm and cjs format
- bundle dependencies to reduce module size
- use package exports to automatically select the appropriate version
- add build step to prepublishOnly
- add build step to ci
@SomaticIT
Copy link
Contributor Author

I added a similar PR on fetch-blob (node-fetch/fetch-blob#107). It allows us to avoid bundling fetch-blob when using esbuild.

If and when merged, we could avoid bundling fetch-blob by changing package.json as follows:

diff --git a/package.json b/package.json
index 7f4726d..668aecf 100644
--- a/package.json
+++ b/package.json
@@ -21,8 +21,8 @@
   },
   "scripts": {
     "build": "npm run build:esm && npm run build:cjs",
-    "build:esm": "esbuild src/index.js --bundle --platform=node --format=esm --outfile=dist/index.mjs",
-    "build:cjs": "esbuild src/index.js --bundle --platform=node --format=cjs --outfile=dist/index.cjs",
+    "build:esm": "esbuild src/index.js --bundle --platform=node --format=esm --outfile=dist/index.mjs --external:fetch-blob",
+    "build:cjs": "esbuild src/index.js --bundle --platform=node --format=cjs --outfile=dist/index.cjs --external:fetch-blob",
     "test": "mocha",
     "coverage": "c8 report --reporter=text-lcov | coveralls",
     "test-types": "tsd",
@@ -66,7 +66,6 @@
     "data-uri-to-buffer": "^3.0.1",
     "delay": "^5.0.0",
     "esbuild": "^0.12.16",
-    "fetch-blob": "^3.1.2",
     "form-data": "^4.0.0",
     "formdata-node": "^3.5.4",
     "mocha": "^8.3.2",
@@ -74,6 +73,9 @@
     "tsd": "^0.14.0",
     "xo": "^0.39.1"
   },
+  "dependencies": {
+    "fetch-blob": "^3.1.2"
+  },
   "tsd": {
     "cwd": "@types",
     "compilerOptions": {

@jimmywarting
Copy link
Collaborator

jimmywarting commented Jul 28, 2021

It feels very unappealing to basically double the hole codebase and start depending on a bundler just to compile everything to cjs and going back. using a module wrapper isn't a good solution either as other dependencies are esm only packages.

Many modules steers away from cjs nowdays.
cjs is considered as legacy and it's discouraged. new code should be using ESM instead. it makes it compatible with Deno and browser without having to use any bundler.

It's also quite unappealing to have a own version of whatwg:streams (coming from fetch-blob if we bundled everything and make it dependency free) as different version of whatwg:streams are not interchangeable with other versions. MattiasBuelens/web-streams-polyfill#82 (comment)

web streams polyfill could be used by other consumers as well.

I would rather suggest a very basic solution that goes something like:

// something.cjs
exports = fetch = (...args) => import('node-fetch').then(module => module.default(...args))

You won't be able to import Request, Response or Headers this way but not everyone needs those.
My opinion is that we should stick with ESM only without a bundler


If you wish to use node-fetch@v3 then you should be using ESM as well... all new code shouldIt is time that we steer away from using it, cjs won't go away unless we stop using it.
if you are stuck with cjs then better use any other ajax library, wait until fetch gets implemented into core or stay with v2.

@LinusU
Copy link
Member

LinusU commented Jul 29, 2021

I think that publishing both CJS and ESM in the package is the wrong way to go. If it's super important to still support the old CommonJS way of importing the module, even though all currently supported Node.js versions supports ESM, I would rather see that we only published as CommonJS.

While there is a small upside in bundling the code before publishing to Npm, I think that we should also consider the downsides. E.g. one can no longer install a specific branch or commit by doing npm install node-fetch/node-fetch#aabbccd...

In this implementation, I bundle both data-uri-to-buffer and fetch-blob into the final result allowing us to be dependency free.

With this change we need to bump and publish a new package any time data-uri-to-buffer or fetch-blob publishes a new version. And if a security vulnerability is fixed in either of them, our users won't get that update until we publish a new version. I think that this is very bad, and I don't really see the upside. What is the benefit of doing this?

In JavaScript codebases, we need to use the async import() which can create a lot of changes.

Why can't you just do import fetch from 'node-fetch' as any normal dependency? 🤔

In TypeScript codebases (with cjs module mode), it is impossible to use node-fetch anymore.

This is easily solved by using "module": "ES2020" instead of "module": "CommonJS". Since every currently supported version of Node.js supports ESM, the ecosystem is moving to it. E.g. both me and Sindre is currently moving over all our packages and are only supporting ESM. And since ESM code can import CommonJS modules there is nothing stopping a project to move to ESM before all their dependencies are updated...


My opinion is that we should stick with ESM only without a bundler ☺️

@loynoir
Copy link

loynoir commented Aug 5, 2021

@LinusU

I think that publishing both CJS and ESM in the package is the wrong way to go.

Just a bit of curious, why do you think it's a wrong way?
I think conditional exports is born for it and else.

My opinion is that we should stick with ESM only without a bundler relaxed

Agree but not agree. As you can not write CJS and ESM at the same time.
You need to choose at least one from

  1. transpiler
  2. bundler
  • Transpiler: typescript
    Pro: I think might be best option, can easily transpile to ESM, CJS, etc.
    Con: Still not support output extension name with .mjs, need time to change codebase (Or maybe --allowJs? haven't try)

  • Transpiler: babel
    Pro: ...
    Con: need to maintain babelrc + its plugins

  • Bundler: rollup
    Pro: ...
    Con: need to maintain babelrc + its plugins

  • Bundler: esbuild
    Pro: out-of-box, no config, no need plugins
    Con: less easy extendable than rollup

So, as I understand why dev drop CJS. I might on my own. To decrease maintainance, I choose esbuild.

But, I still want to say, please re-consider. Support both as we are still not there yet.

@jimmywarting
Copy link
Collaborator

@loynoir i feel strongly against bundeling whatwg streams and having a own instance (of everything basically) that are not interchangeable just b/c of this reason alone:
MattiasBuelens/web-streams-polyfill#82 (comment)

Do you have any arguments against that?

@LinusU
Copy link
Member

LinusU commented Aug 5, 2021

@loynoir

Just a bit of curious, why do you think it's a wrong way?

I don't see any benefit of doing this, over just publishing a CommonJS module, since ESM can import CommonJS modules. If you know of any benefits I would be happy to hear them!

The downside is that you are shipping your entire module twice, which means (apart from doubling the size) that there is a risk that you'll load two different but very similar versions of the package at runtime. This can then lead to the "dual package hazard" which can cause runtime breakage. e.g. https://github.com/GeoffreyBooth/dual-package-hazard

Agree but not agree. As you can not write CJS and ESM at the same time.
You need to choose at least one from
[...]

I don't really understand this 🤔

Node.js supports importing ESM modules natively, you do not need neither a bundler or a transpiler?

Support both as we are still not there yet.

The stable 2.x line is still fully functional and this package is still in beta. I think it's reasonable that people upgrade to 3.x at the same time when they make sure that they are running a supported versions of Node.js and switches to ESM.

@loynoir
Copy link

loynoir commented Aug 5, 2021

Hi, @jimmywarting.

Do you have any arguments against that?

I do not want to argue with anyone, as I said I totally understand the movement. And I'm also a big fan of ESM.

i feel strongly against bundeling whatwg streams and having a own instance

Agree. But...😂

$ esbuild --format=cjs --outdir=dist/cjs ./src/*js ./src/errors/*js ./src/utils/*js
dist
└── cjs
    ├── body.js
    ├── errors
    │   ├── abort-error.js
    │   ├── base.js
    │   └── fetch-error.js
    ├── headers.js
    ├── index.js
    ├── request.js
    ├── response.js
    └── utils
        ├── form-data.js
        ├── get-search.js
        ├── is.js
        └── is-redirect.js
> require('./dist/cjs')
Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: fetch-blob/index.js

Just lack of energy to find out all ESM only dependencies, and fork every of them. 😂

I don't think bundler is a good idea, but at least it's acceptable and quick workaround, for me.

Hi, @LinusU

Node.js supports importing ESM modules natively, you do not need neither a bundler or a transpiler?

If I rememeber correctly, the last time I check jest source code, jest use vm, and node.js
vm ESM support is still experimental --experimental-vm-modules.

So, maybe it's not native enough.

Hi, @SomaticIT

Maybe let esbuild do no bundeling,

esbuild --format=cjs --outdir=dist/cjs ./src/*js ./src/errors/*js ./src/utils/*js

After fetch-blob is not ESM only.

> require('./dist/cjs')
Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: fetch-blob/index.js

@hjdhjd
Copy link

hjdhjd commented Aug 5, 2021

Support both as we are still not there yet.

The stable 2.x line is still fully functional and this package is still in beta. I think it's reasonable that people upgrade to 3.x at the same time when they make sure that they are running a supported versions of Node.js and switches to ESM.

Well…I’d be happy to stick to 2.x if you’d backport some of the features in 3.x I’m using. You‘re effectively making the same argument here about maintaining multiple codebases if you intend to keep 2.x and 3.x (once it comes out of beta) going, no?

Also - I think one of the scenarios that’s being missed is situations like mine: some of my code that uses node-fetch are plugins to other projects that don’t yet support ESM. You’re forcing me to version lock in a situation I have little control over, as I’m operating in a broader ecosystem that’s slower to implement ESM support. Now my choices are:

  1. Switch away from node-fetch. (not my preference)
  2. Lock support to beta9 and hope that there’s no major bug or security implication that needs to be resolved in the future until I (and the broader ecosystem I’m writing plugins for) can all upgrade to ESM. (what I’ve opted to do for the time being).
  3. Revert to 2.x and lose features that I rely on, and introduce other potential problems.

We all recognize it’s beta, and stuff happens and major breaking changes is part of “stuff”. I can make the argument both ways. 😄

@jimmywarting
Copy link
Collaborator

jimmywarting commented Aug 5, 2021

have you tried just doing:

const fetch = (...args) => import('node-fetch').then(module => module.default(...args))

...to see if it works?

@SomaticIT
Copy link
Contributor Author

SomaticIT commented Aug 11, 2021

have you tried just doing:

const fetch = (...args) => import('node-fetch').then(module => module.default(...args))

...to see if it works?

I think it should work but if exported by node-fetch, you would have a different module exports (no more Headers, Request, Response, etc. classes exported).

@loynoir i feel strongly against bundeling whatwg streams and having a own instance (of everything basically) that are not interchangeable just b/c of this reason alone:
MattiasBuelens/web-streams-polyfill#82 (comment)

Do you have any arguments against that?

Bundling of dependencies is totally optionnal. I used it because, web-streams-polyfill is very big when installed because it contains polyfills for every runtime versions. I thought that we could use only the good one in our environment. I can remove it from bundling if you want.

It feels very unappealing to basically double the hole codebase and start depending on a bundler just to compile everything to cjs and going back. using a module wrapper isn't a good solution either as other dependencies are esm only packages.

Many modules steers away from cjs nowdays.
cjs is considered as legacy and it's discouraged. new code should be using ESM instead. it makes it compatible with Deno and browser without having to use any bundler.

It's also quite unappealing to have a own version of whatwg:streams (coming from fetch-blob if we bundled everything and make it dependency free) as different version of whatwg:streams are not interchangeable with other versions. MattiasBuelens/web-streams-polyfill#82 (comment)

web streams polyfill could be used by other consumers as well.

I would rather suggest a very basic solution that goes something like:

// something.cjs
exports = fetch = (...args) => import('node-fetch').then(module => module.default(...args))

You won't be able to import Request, Response or Headers this way but not everyone needs those.
My opinion is that we should stick with ESM only without a bundler

If you wish to use node-fetch@v3 then you should be using ESM as well... all new code shouldIt is time that we steer away from using it, cjs won't go away unless we stop using it.
if you are stuck with cjs then better use any other ajax library, wait until fetch gets implemented into core or stay with v2.

There is no doubling of codebase, you keep the same source code for both exports.

Unlike what many people thinks, bundling is a very good practice for performance (even without external dependencies). The public package is tinier and it loads faster (less disk operations).

Having both CJS and ESM exports allows all existing consumers to keep their codebase without changing any line of code. I think that it's very important when you maintain a library which is very popular like node-fetch.

I'm also a big fan of ESM but I think that we should be concerned of existing codebases before making such a breaking change.

The stable 2.x line is still fully functional and this package is still in beta. I think it's reasonable that people upgrade to 3.x at the same time when they make sure that they are running a supported versions of Node.js and switches to ESM.

We should think of when v3 will be stable. You will have a lot of issues of people upgrading their dependencies and having trouble with their existing codebases.


Thanks for all you comments.
I can upgrade this PR to avoid bundling dependencies if you think it's preferable.

@jimmywarting
Copy link
Collaborator

jimmywarting commented Aug 12, 2021

I think it should work but if exported by node-fetch, you would have a different module exports (no more Headers, Request, Response, etc. classes exported).

You can still load headers, req, res etc but it has to be done async. Another way if you still have a large codebase is to bootstrap your application to load node-fetch first and then run your own application

// main `./bootstrap.js` file in cjs format

// Load node-fetch first.
import('node-fetch').then(mod => {
  // Assign fetch to global namespace (could also assign it to a
  // cjs factory that can be require'd if you don't want to pollute global namespace)
  const {default: fetch, ...rest} = mod.default
  Object.assign(globalThis, rest, { fetch })

  // Run your main legacy application that is built entirely in commonjs format.
  import('./index.js')
})

(ofc i know this dose not work for everyones needs but cjs and esm can still go exist with each other - you do not have to update your entire codebase to esm in one go to be able to use packages that have been targeted for ESM only)

I can upgrade this PR to avoid bundling dependencies if you think it's preferable.

That won't solve the fact that fetch-blob is a ESM only package. If we switch to cjs too then we can't load the fetch-blob cuz esm packages are loaded asynchronous by nature. You would have to either bundling everything or nothing.

@jimmywarting
Copy link
Collaborator

One use case for cjs even in modern node environment is server apps build with webpack.

It is pretty common to compile react app for node to enable server-side rendering simply because jsx syntax is not native JS feature and webpack one of the most popular tools for that.

But webpack also transforms native imports to own implementation that uses require() under the hood. Which makes node app bundled with webpack incompatible with esm module imports.

Yes, it is possible to include node-fetch into bundle and transform it to cjs on app level in cases like this but:

  1. it is not recommended in general for node target (external package might use binary files, etc.)
  2. it is very unreliable because end-user need to analyse which esm dependencies node-fetch has and whitelist it as well from externals

I wish webpack can support esm compilation target so imports can be left untouched but it is still under development and does not seems to be released soon.

Originally posted by @gbiryukov in #1226 (comment)

@Richienb
Copy link
Member

One use case for cjs even in modern node environment is server apps build with webpack.

That is a problem with webpack, not with node-fetch. Since every supported version of Node.js supports ESM modules, there is no point adding CommonJS bundles.

@gbiryukov
Copy link

Indeed. The question is how many users of webpack will face this issue and spend their time to find workaround (which is not extremely simple due to couple of reasons).

Personally to me it seems like majority of the users of node-fetch are people with universal apps which means 90% chance of having webpack in the pipeline.

Cjs build or at least simple guide how to use node-fetch with webpack will eliminate a lot of friction

@LinusU
Copy link
Member

LinusU commented Aug 16, 2021

If people are building with webpack, will it not simply inline this entire module, which will eliminate the problem altogether?

Also, webpack has support for outputting a bundle that uses import/export instead of require

@gbiryukov
Copy link

with webpack, will it not simply inline this entire module, which will eliminate the problem altogether?

when app bundled for target node it is good practice to do not bundle third parties because some of them can use/install binaries, can do something important for environment in port-install etc. There are also couple of reasons for library authors.

Also, webpack has support for outputting a bundle that uses import/export instead of require

true but this feature only available in experimental mode and only for v5 users which have pretty small adoption.

I understand intention to keep things simple and forget about cjs (and other not most up to date stuff) but just makes such a pain to use node-fetch for many people. It also seems to be source of many similar issues in this repo so at would be great at least to document how to use it in cjs build

@jimmywarting
Copy link
Collaborator

Personally to me it seems like majority of the users of node-fetch are people with universal apps which means 90% chance of having webpack in the pipeline.

If you are using webpack to bundle stuff then it most likely mean frontend apps? in which case window.fetch should be used instead of node-fetch. (something like isomorphic-fetch)
Targeting Deno? then use Deno's global fetch api. It is a waste to include something that already is supported

least to document how to use it in cjs build

That is what we are trying to achieve with #1236

@gbiryukov
Copy link

I've added some context above

It is pretty common to compile react app for node to enable server-side rendering simply because jsx syntax is not native JS feature and webpack one of the most popular tools for that.

So the need for node-fetch in webpack bundled node app is to mimic browser environment and make server-side rendering work same way as in the browser

@jimmywarting
Copy link
Collaborator

I suppose you are talking about native react (for phones) and not the web version of react?

@gbiryukov
Copy link

I am talking about web version of react. It uses own syntax (JSX) that is not part of ES6 so it needs to be transformed into ES6 before running in both browser and node environments

@Richienb
Copy link
Member

Richienb commented Aug 21, 2021

During this transformation process, Webpack would transpile node-fetch to CommonJS so that it can run in older browsers. In fact, it should be noted that this is node-fetch.

@jimmywarting jimmywarting changed the title feat: bundle module and use package exports chore(cjs/ems): Bundle module and use package exports Aug 21, 2021
@vjpr
Copy link

vjpr commented Aug 31, 2021

This is a bad decision. ES modules are not ready for prime-time. And its not trivial to shift large projects to ES modules.

It just cuts off new bug fixes and features to non-ESM packages.

Take the expo project for example. xdl > @expo/dev-server > node-fetch. You are basically asking them to migrate the huge xdl codebase to ESM in order to get bug fixes for a small dependency of a dependency.

And if you use any kind of import/require hooks, well the API for this is not stable and will change soon. So if you rely on these features, not only is it a lot of dev work, you simply cannot migrate because ESM is not ready.

Same discussion here regarding Sindre's packages which probably kicked off this chaos.

Conditional exports was built into Node.js to avoid this pain...and every library just ignores it.

@vjpr vjpr mentioned this pull request Aug 31, 2021
@justingrant
Copy link

FYI, AWS Lambda (and other platforms based on Lambda, e.g. Netlify functions) still doesn't have ESM support in its Node14 implementation. So anyone building Lambda functions will need to package their code as CJS. See https://stackoverflow.com/questions/66676555/nodejs-14-x-native-aws-lambda-import-export-support/66688901#66688901 for updates.

When I have time, I'm going to investigate using esbuild (probably via serverless-esbuild because my Lambda functions are built with the serverless framework) to transform ESM code into CJS so that AWS Lambda will run it. I've never used esbuild before so I'm not sure this will work, but it seems easy enough to try it.

Sadly, I suspect that the ESM-only approach that node-fetch and Sindre are doing is what will be needed to force the rest of the ecosystem (looking at you, AWS!) to get their own ESM support shipped. It's like a big game of tech chicken, and hopefully ESM will win the game before everyone crashes! :-)

@coyoteecd
Copy link

Another use case that brought me here is compiling to commonjs for unit tests only, to be able to spy on exported functions (see discussion in jasmine/jasmine#1414 (comment) and https://jasmine.github.io/pages/faq.html#module-spy)

@LinusU
Copy link
Member

LinusU commented Sep 1, 2021

@justingrant I would recommend you to bundle your code before shipping to Lambda in any case, it makes a ton of difference (in my experience) with the size of the Lambda bundle, and the cold boot time. You can use e.g. ncc to do this without much setup at all!

For Lambda I also think it's very good that the ecosystem is aggressively moving to ESM, because without that I don't think that AWS will implement support for it anytime soon. Many packages not working without it will hopefully pressure them to fix ESM support (which by the way would be very easy for them, if you dump their startup code you can see that it basically is just to replace a require(...) with an import(...) call, since they are loading your code dynamically already).

@lostpebble
Copy link

Honestly this is the most short-sighted thing I think I've ever witnessed in the JavaScript space. All these people pushing ESM as if its "for the greater good" ... I can't think they have ever worked on an even slightly large Node.js project with diverse dependency trees. Or realize that Webpack itself is still broken when trying to make your project fully ESM as output...

Thanks to you guys- I have spent the past week of wasted time trying to resolve these require() doesn't work with ESM module errors and have come to almost no real progress at the end of it and its literally driving me crazy. That's just me- about 40 hours of wasted time because of these folks who I can only think are either ignorant to real-world JavaScript use, or are simply unempathetic and couldn't care about the now probably hundreds of thousands of wasted productivity hours which will very soon be a whole lot more when people start updating their dependencies to these new versions. (This is especially ironic to me when you see that the guy who is pushing this the hardest has "Wants more empathy & kindness in open source" in his Github bio)- @sindresorhus please, I beg of you to take heed of your own ideals there!

And its often not our choice at all- we might just update a single module for a security release- and that module has now made use of a new version of a module (node-fetch for example) and now our entire app cannot run on Node.js anymore unless we somehow find a way to package the entire thing as ESM- which is not possible for many people who rely on things like Webpack to package their apps. This is an extremely premature decision and it is seriously going to hurt developers.

I can appreciate trying to move JavaScript forward- we all want to work with pure modules and modern JS too- but this whole thing is at least a year too early, modules has only just come out of experimental in Node.js (relatively speaking) and Webpack 5 doesn't even support modules (its under an experimental flag, but even then it fails to package your code as ESM in many cases). This is literally a dead end for users.

@jimmywarting
Copy link
Collaborator

I can't think they have ever worked on an even slightly large Node.js project with diverse dependency trees

we have...

Or realize that Webpack itself is still broken when trying to make your project fully ESM as output

That's webpacks problem. We considered packing node-fetch our self to have dual suport but the conns outweighed the pros

I have spent the past week of wasted time trying to resolve these require()

v3 have only been out for 2 days...

And its often not our choice at all- we might just update a single module for a security release- and that module has now made use of a new version of a module (node-fetch for example)

you often have a choice, you can stick with v2.x if you like, it's very stable, it has fixed many bugs and haven't been touched much
We will continue to backport and fix security issues with the version 2,

@lostpebble
Copy link

That's webpacks problem

This is the lack of empathy and understanding of the JavaScript ecosystem as a whole that I'm talking about. I don't think you realize just how many projects have Webpack as a core part of their packaging pipeline. This is why I'm saying its premature, there are options given to you to make this transition easier on everyone (such as pointed out by @vjpr, conditional exports)- but you have decided to take the "scorched-earth" policy instead.

v3 have only been out for 2 days...

node-fetch is a recent one that I have updated- but the biggest issues were most of @sindresorhus modules- which is literally thousands which are imported in all kinds of nooks and crannies of the JavaScript ecosystem. Including so many little "one-line" packages such as is-plain-obj etc.

you often have a choice, you can stick with v2.x if you like, it's very stable, it has fixed many bugs and haven't been touched much. We will continue to backport and fix security issues with the version 2,

While I appreciate that... as I said we often don't make that choice directly ... This is why I ask if you have ever really worked on large JS projects, possibly monorepos that link up and use 100s of diverse dependencies, some of which have decided to update a dependency to a version that uses ESM (unknowing to the library author how that might effect downstream users because there won't be any errors popping up when he bundles his library) which now causes all projects which update (sometimes even a minor version, because their API hasn't changed at all) to suddenly be faced with all kinds of errors. Now imagine that this kind of thing happens- but its 3 dependencies deep. This is a huge mess, and I don't think its fair on users who are simply trying to be productive to tell them to now chase down all these version numbers and fix everything, or make PRs to all his dependent projects to back-port needed functionality to the version that makes use of an older non-ESM dependency... especially when popular tooling, such as Webpack 5 (probably the most used JS bundling tool by far) still does not support outputting our projects to ESM to make this transition easier.

@loynoir
Copy link

loynoir commented Sep 2, 2021

@Richienb

That is a problem with webpack, not with node-fetch. Since every supported version of Node.js supports ESM modules, there is no point adding CommonJS bundles.

Please. Please. Please.
Make contribution to Node.js

  • make --experimental-vm-modules from experimental to stable.
    Jest is running under vm module.
  • make esm_loaders stable
  • ...

I love and using ESM, but honestly ESM is not totally ready.

I totally agree on your foresee of the future of ESM. But no one foresee 2020.
Just currently, not a proper time for ESM-only, but dual-module.

@loynoir
Copy link

loynoir commented Sep 2, 2021

Although I find a way out, but

  • The ESM tool ecosystem is not complete
  • ESM-only under vm context is experimental, --experimental-vm-modules & jest
    I don't think it's a proper time for ESM-only, but dual-module

@jimmywarting
Please image how many current hours and future hours, around the whole world used, within node-fetch dependency network, just because you decide to ESM-only over dual-module.
Please re-consider.

@loynoir
Copy link

loynoir commented Sep 2, 2021

@LinusU

I don't see any benefit of doing this, over just publishing a CommonJS module, since ESM can import CommonJS modules. If you know of any benefits I would be happy to hear them!

Actually there is one.
The full ESM-only ecosystem haven't grown up.
Just image how many time whole world downstream user will use, if popular upstream library decide to ESM-only while the whole ecosystem haven't ready.
One reason, but much. But human time are precious, which make one reason weight enough?

Node.js supports importing ESM modules natively

  • --experimental-vm-modules
  • esm_loaders: "This API is currently being redesigned and will still change."

I love and fully using ESM, but ESM is not totally ready. But it is not yet a ESM-only period.

@SomaticIT
Copy link
Contributor Author

SomaticIT commented Sep 2, 2021

Hello everyone,

Just some figures from npm:

  • 21 449 dependant packages
  • 21 653 327 weekly downloads

It's only npm. Based on the number of dependant packages and weekly downloads, you can estimate that there are millions of projects which uses node-fetch.

So this decision is impacting millions of developers around the world who will encounter an unexpected issue when migrating to node-fetch v3. They will have to spend multiple hours to understand the issue and find a workaround that works in their specific environment.


This decision will cost millions of hours (and dollars) in the javascript development ecosystem. You will directly impact the job of millions of developers and companies. Finally, all the time lost on node-fetch is not spent innovating.

This could become the worst decision in the node.js ecosystem in 2021!


And this decision is taken only because you don't want to introduce a transpiler (here esbuild).

In this PR, I suggest to not-only transpile the code but also bundle it (for performance). If you don't like bundling, we could only transpile the code. Transpilation is a safe process, it can be tested during the CI and it will not take you too much time.

Do you agree if I update this PR to make a new suggestion with only transpilation (no more bundling) and CI testing?


I'm also a huge fan of ESM and my conviction is that it's the future of javascript. I think it should be used on every new projects.

But here we talk about maintaining a globally used library. You can't just impose such a breaking change to millions of people because it's your (own) conviction. Please be humble and reconsider your convictions.


Edit: you should also think about TypeScript users who could not mix CJS require and ESM imports. They are just stuck, they will never be able to update node-fetch (no more bug fixes, security patches, etc.).

@RedGuy12

This comment has been minimized.

@loynoir
Copy link

loynoir commented Sep 2, 2021

@jimmywarting @SomaticIT

Should the title to Re-add dual module support? Add using bundler is just an approach to inline ESM only node-fetch/fetch-blob

If concern about release size, can also use postinstall field, right?

@jimmywarting jimmywarting changed the title chore(cjs/ems): Bundle module and use package exports chore(cjs/esm): Bundle module and use package exports Sep 2, 2021
@jimmywarting
Copy link
Collaborator

the release size is not the only thing on my mind. it's about not having 2 instances of whatwg:streams or anything else for that mater

MattiasBuelens/web-streams-polyfill#70 (comment)

A ReadableStream created from version 2.1.1 can not pipe data to a WritableStream constructed by version 2.1.2
if we would have a dual model support with commonjs then it means that we need to reintroduce a bundler and package everything into one file and have a own instances of ReadableStream, a own instances of fetch-blob, base64 parser and every other dependency that we depend on.
that duplicates all code, it creates own instances of everything and it would be incompatible with a stream that you yourself are using.

@LinusU
Copy link
Member

LinusU commented Sep 2, 2021

@SomaticIT is there anything preventing you from staying on 2.x as long as you need CommonJS, and then upgrading to 3.x when you are switched over to ESM?

As was said before:

[...] you can stick with v2.x if you like, it's very stable, it has fixed many bugs and haven't been touched much
We will continue to backport and fix security issues with the version 2,

@loynoir
Copy link

loynoir commented Sep 2, 2021

@jimmywarting
Not very understand. Is node-fetch facing problem ESM and CJS state references is not same?

Maybe just rename node-fetch@3.x as new channel or something like node-fetch@next / @node-fetch/foo, get away from <PM> install node-fetch.

As it's is breaking update for current ecosystem.

FYI, I'm using pnpm. I updated without see any warning weeks ago..

@SomaticIT
Copy link
Contributor Author

@jimmywarting, There is no problem to stay with unbundled dependencies. We just need to set them as external when running esbuild.

@LinusU, Yes for 3 reasons:

  1. v2 typings are not correct (I collaborated with you to build the v3 typings the right way).
  2. I wonder if v2 will continue to be updated with bug fixes and security patches.
  3. In my company, we have automated processes that ensures that all our dependencies stay up to date and block our CI pipelines if they are not. This is something we guarantee to our customers. Ignoring a library like node-fetch which communicates over the network is a real threat and could create a legal problem.

@LinusU, Moreover, I'm not the only one to considerate. As I said before, there are millions of projects which uses node-fetch and many of them (like some of mines) are big projects with 100k+ lines. It could take years for such projects to migrate to ESM.

@LinusU
Copy link
Member

LinusU commented Sep 2, 2021

  1. v2 typings are not correct (I collaborated with you to build the v3 typings the right way).

Could we fix this in the 2.x branch?

  1. I wonder if v2 will continue to be updated with bug fixes and security patches.

As have been stated in this very thread, we are open to fixing bugs and security issues.

  1. In my company, we have automated processes that ensures that all our dependencies stay up to date and block our CI pipelines if they are not. This is something we guarantee to our customers. Ignoring a library like node-fetch which communicates over the network is a real threat and could create a legal problem.

As I'm sure you are aware, many other maintainers, including Sindre with his 1000+ packages, are switching to ESM only. I think that you will need to reconsider this approach if you want to stay with CommonJS...

@LinusU, Moreover, I'm not the only one to considerate. As I said before, there are millions of projects which uses node-fetch and many of them (like some of mines) are big projects with 100k+ lines. It could take years for such projects to migrate to ESM.

I think that the same thing I said to you applies to other projects: is there anything preventing them from staying on 2.x as long as they need CommonJS, and then upgrading to 3.x when they are switched over to ESM?

@gbiryukov
Copy link

in case of universal apps (that runs both in browser and in node) v3 is very desirable simply due to better compliance to the the spec so all features can be safely used like Response.error() method that is missing in v2 but available in v3

@SomaticIT
Copy link
Contributor Author

Hello everyone,

Just to let the community knows, after a lot of issues in my existing projects, I created a wrapper of node-fetch to use in commonjs environments.

The goal of this package is to help commonjs users who have difficulties to migrate their existing codebase to v3 while avoiding to increase the work for node-fetch authors.

You can find it here: touchifyapp/node-fetch-cjs.

If you are able to migrate to the official v3 release of node-fetch, I highly recommend to use the official node-fetch. This package is built to help users who could not migrate easily.

This repository is automatically updated when a new version of node-fetch is released

@melissarh57
Copy link

I'm wondering if anyone has any suggestions for if there is a way to use node-fetch v3 in my specific situation. I am currently on a project using webpack which outputs cjs because we are stuck using node 14 in an aws lambda. esm is only experimental in node14, so I can't switch the whole project over to esm. Just curious if there are any other options besides stay with v2 or switch to another library.

@jimmywarting
Copy link
Collaborator

@melissarh57 ESM should be supported all the way from v12.17 and cjs should be able to load esm only packages using async import.

maybe this solution can work for you #1279 (comment) ?

@loynoir
Copy link

loynoir commented Apr 21, 2022

https://nodejs.org/en/blog/announcements/v18-release-announce/#fetch-experimental

In Node.js 18, an experimental global fetch API is available by default.

$ node
Welcome to Node.js v18.0.0.
> fetch
[AsyncFunction: fetch]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet