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

ESM only breaks everything #1263

Open
yinzara opened this issue Sep 1, 2021 · 85 comments
Open

ESM only breaks everything #1263

yinzara opened this issue Sep 1, 2021 · 85 comments
Labels

Comments

@yinzara
Copy link

yinzara commented Sep 1, 2021

Following the conversation in:
sindresorhus/meta#15 (comment)

PLEASE PLEASE PLEASE, don't do this yet.

ESM is not ready to have a library as major as node-fetch go ESM only. The workarounds you list in the README will not work in all cases.

node-fetch is one of the most mocked libraries in existence. This move will cause MANY projects that's sole purpose is to mock node-fetch to cease to work with no real way to work around the problem.

PLEASE reconsider.


Please be civilized, don't post harsh comment or hatred to node-fetch or that ESM-only is bad.
...or we will lock this once again. We are here do discus solutions
Throw out your frustrations elsewhere. If it has no valuable meaning, then don't post anything at all.

We spend lots of work trying to be spec compliant and fetch isn't a small piece, it involves many web-components.
ESM is one of many spec'ed things such as whatwg:stream, AbortSignal, Blob, File, FormData, EventTarget, FetchEvent, that builds up the hole fetch API for what it's
Don't say stuff like "This requires lots of work to be converted into ESM" that is simply not true. There are solutions that don't involve converting your entire codebase to ESM while still being able to import ESM-only packages from CJS.
Think how many hours you have saved because you could simply import something that is free & open source that others have put theirs heart and soul into and reviewing lots of commits and helping others.
@yinzara yinzara added the bug label Sep 1, 2021
@herberthobregon
Copy link

herberthobregon commented Sep 2, 2021

ESM is the future but i guess the solution is to add "exports" in the package.json for commonjs and esm (?)

Typescript can be the solution to transpile correctly. (Currently Typescript breaks with v3)

ooooorrrrr

keep use "node-fetch": "^2.6.1"

@Taelyn
Copy link

Taelyn commented Sep 2, 2021

Honestly i find that a lazy/stupid/easy solution to the issue. "just stay on v2.6.1"

There was no need to break everyones app then the author thought it was smart to do so

For me dropping this moveing to another library. The owners might need to go learn some skills ;)
This is something you dont do with products.

@nannal
Copy link

nannal commented Sep 2, 2021

It's a community splitting decision, like Python 3 vs 2.7, which has been an on-going battle for over a decade, I wouldn't be surprised to see the same happen here unless changes are made.

@LinusU
Copy link
Member

LinusU commented Sep 2, 2021

[...] , don't do this yet.

@yinzara we will continue to fix critical bugs and security issues in 2.x, so there should be nothing preventing anyone to stay on node-fetch 2.x until they are ready to switch to ESM.

There was no need to break everyones app then the author thought it was smart to do so

@Taelyn I don't see how any app was broken since this was behind a major version bump. Did you have your version of node-fetch set to "*"? 🤔

It's a community splitting decision, like Python 3 vs 2.7, which has been an on-going battle for over a decade, I wouldn't be surprised to see the same happen here unless changes are made.

@nannal I think that this is why it's important for the ecosystem to migrate to EMS as soon as possible. The problem with Python 2/3 was that many major libraries kept supporting only Python 2, or kept supporting both and thus didn't encourage anyone to update.

@yinzara
Copy link
Author

yinzara commented Sep 2, 2021

@yinzara we will continue to fix critical bugs and security issues in 2.x, so there should be nothing preventing anyone to stay on node-fetch 2.x until they are ready to switch to ESM.

Why do this at all yet? That means you now have to maintain two branches of code for all security issues and critical bugs. What if there are any official changes to the fetch API? What about a non-critical bug that still affects your users? 2.x users will then be SoL.

And while it may be true, there will be an enormous amount of people who attempt to switch to 3.x simply because it's available and all of the automated dependency management solutions will attempt the upgrade. It will cause a massive amount of people's code to break (but only at runtime as even TypeScript code will still compile without breaking) and then file issues with package maintainers (and I'm sure with this library).

I'm one of the maintainers of "jest-fetch-mock". There is literally nothing we can do to fix the library until Jest solves the mocking issue which thus far I have no idea how it's possible with the current state of the ecosystem for them to do that. We are now going to get constant people who file issues with our library saying it doesn't work with node-fetch 3.x and we're going to have to spend time responding to every user over and over again why there's no way for us to support it.

ESM is not ready to be the only option for a library as major as "node-fetch". There are far too many things still unsupported across the ecosystem that are currently unfixable (or have only horrible work arounds like using the "--experimental-loader" flag with Node).

PLEASE PLEASE PLEASE just cross transpile and include the CJS binary and abandon the 2.x branch. It takes almost no effort for you guys to do this, and will save the entire community from a horrible breaking failure that there are no solutions to right now. You can have a 4.x break that's ESM only in some time, when the major frameworks now function with ESM without huge work arounds.

But right now this is going to be horrible for sooooo many.

The entire reason the package.json syntax to support both ESM and CJS were introduced was to make conversions easy.

See the above linked debate. There are very serious (and I believe completely valid) arguments about why the entire community should stop implementing ESM as a whole nevermind stop strong arming it.

@foxt
Copy link

foxt commented Sep 2, 2021

@herberthobregon:

ESM is the future

It might be the future, but it's not the present. If your library makes breaking changes between versions that requires people make large changes to their project's architecure, you're probably doing something wrong.

@LinusU:

I don't see how any app was broken since this was behind a major version bump. Did you have your version of node-fetch set to "*"? 🤔

While unless that was the case, it won't break existing apps, but people who are used to just typing npm i node-fetch are gonna get confused why the library suddenly stopped working.

node-fetch is the type of library users expect to just work, and is integral to many types of use cases. I'm not even sure why we need such big changes, the only updates I see node-fetch requiring is bug fixes & updating with the infrequent updates to the fetch spec, and looking at the commit history, that seems true. Case of maintainers getting bored?

IMHO, when a package is used by 3.3 million projects, feel like maintainers should at least try their best to support all 3.3 million of those projects (Now I see sometimes breaking changes do need to be made, and of course, you can't support all of them, but this change doesn't seem required)

@mhf-ir
Copy link

mhf-ir commented Sep 5, 2021

That's stupid idea for removing CJS. CJS is main development stream of even modern applications in Node.js (not frondend apps)
Multiple distribute.
https://github.com/unjs/ufo#install

CommonJS never will be deprecate or drop, it's different approach
nodejs/help#2267

@herberthobregon
Copy link

@mhf-ir

That's stupid idea for removing CJS. CJS is main development stream of even modern applications in Node.js (not frondend apps)

Moderate how you express yourself. Just stay in version 2, never use the asterisk in your package.json 🤷‍♂️ this is bad practice.

@mhf-ir
Copy link

mhf-ir commented Sep 6, 2021

@herberthobregon of course using asterisk in version is bad practice but, developing new version that drop stable core of node.js is not great choice.

Latest node.js version
https://nodejs.org/dist/latest-v16.x/docs/api/modules.html#modules_modules_commonjs_modules

Screenshot from 2021-09-06 04-43-25

That's green bar with Stable

node-fetch drop stable core feature/mature of node.js

@meltifa
Copy link

meltifa commented Sep 6, 2021

node doesn't plan to deprecate cjs, so why must node-fetch v3 force users to deprecate cjs?

This makes absolutely no sense.

@jimmywarting jimmywarting changed the title ESM only breaks EVERYTHING ESM only breaks everything Sep 6, 2021
@martinrotter
Copy link

martinrotter commented Sep 6, 2021

package.json

There are even applications which do not have "package.json". Some peeps call "node.exe" directly and pass path to JS script as parameters etc.

3.0.0 broke stuff for me too.

@jimmywarting
Copy link
Collaborator

jimmywarting commented Sep 6, 2021

There are even applications which do not have "package.json".

Without a package.json then you can do: node --input-type=module file.js

@Ghazgkull
Copy link

Ghazgkull commented Sep 7, 2021

In addition to the other issues mentioned above, I'd like to call out that this breaking change causes a problem for projects that have manual or automated processes to update NPM dependencies on a regular cadence.

It's always possible that major version bumps will require adoption and everyone's update processes need to take this into account. But a breaking change like this which can't be adopted is an entirely different matter.

Teams are now required to maintain tribal knowledge around this package (don't update to v3!) or to update their automated dependency upgrade processes to specifically code around this package. As I find myself in the latter camp, sitting here writing scripting to exclude this package, I would like to ask the maintainers to please re-consider doing the right thing for the community. Please either restore CJS functionality for this repo or consider creating a new project for the ESM-only version.

@jimmywarting
Copy link
Collaborator

jimmywarting commented Sep 7, 2021

@Ghazgkull You shouldn't use "node-fetch": "*" or automatically update major versions, there is a reason to semantic versioning. and major versions is breaking change.

there are ways you can still depend on ESM-only packages in cjs without much effort #1266 (comment)

@yinzara
Copy link
Author

yinzara commented Sep 7, 2021

@Ghazgkull You shouldn't use "node-fetch": "*" or automatically update major versions, there is a reason to semantic versioning. and major versions is breaking change.

there are ways you can still depend on ESM-only packages in cjs without much effort #1266 (comment)

"Without much effort"

I completely disagree. TypeScript projects cannot use ESM at all yet unless your project is an ESM project itself.

microsoft/TypeScript#43329

There is definitely a TON of effort involved and it requires a complete change to how you use the library.

And every automated upgrade framework (RenovateBot, Greenkeeper etc) automatically attempt to upgrade major versions unless you specifically configure them not to do that as they're intended to be used with a test suite to verify the changes work with a new version. Most libraries do not make changes that completely change how you must use the library even with major version changes (usually the breaking change is something you can fix in a short time period).

The ESM only shift is NOT that. It requires a ton of effort and the ecosystem cannot yet support it as many of the major frameworks do not yet fully support ESM (see all my above comments and linked issue).

@Ghazgkull
Copy link

Ghazgkull commented Sep 7, 2021

This kind of defensiveness from the maintainers (see strawman above) isn't productive.

package.json files and package-lock.json files are used by professional software developers to pin the versions of NPM libraries like this one. It is an industry best practice to update the versions pinned in those manifests on a regular cadence. To keep up to date not only with security fixes but also to keep in sync with the community around those libraries. For example, if someone came here asking a question about some old feature from v1 of this project, the community knowledge around that old version might be lost and it might be difficult to find help.

Anyway, the proposed solution ("Just don't upgrade. LOL. 4head.") leaves what we call a "rake" in the codebase of any projects using this library. Anyone who tries to upgrade their pinned library versions - via manual or automated processes - will "step on the rake" and find that they're broken and need to revert.

To the maintainers: Please remain civil and try to listen to what the community is saying.

@Taelyn
Copy link

Taelyn commented Sep 7, 2021

Ye so far they have done nothing more then repeating the same.
Reminds me of a guy at my job. Exaclty the same, does what he thinks is cool
doesnt think about it, doesnt want feedback. In the end it turns out we where right :D

Same seems to happen here...

@mattbishop
Copy link

mattbishop commented Sep 7, 2021

Perhaps node-fetch should use CJS for the codebase but provide an MJS file that exports the API for ESM users? This would support those use cases that need ESM (like Deno) while keeping the current user base onboard for the coming years while Node moves their support from Experimental to Stable.

There's a nice how-to at the end of this article:

https://redfin.engineering/node-modules-at-war-why-commonjs-and-es-modules-cant-get-along-9617135eeca1

@jimmywarting
Copy link
Collaborator

Deno would not have any benefit of node-fetch what so ever. they have fetch built in

@Ghazgkull

This comment has been minimized.

@jimmywarting

This comment has been minimized.

@RedGuy12

This comment has been minimized.

@jimmywarting

This comment has been minimized.

@darthtrevino
Copy link

darthtrevino commented Sep 9, 2021

I did this same thing with react-dnd a while back and exported ESM only. Unfortunately Node just isn't at the point where we can just use ESM within .js files, and it caused me a lot of grief. I feel your team's pain right now.

@NGPixel
Copy link

NGPixel commented Sep 13, 2021

Maintainers seem to live in a dream bubble completely disconnected from reality. You can't expect users to rework their entire project because 1 small library decided to go the ESM-only route and completely block the use of a core STABLE feature named CJS.

This library is used in much larger projects / libraries that simply CANNOT be refactored using any of the solutions you suggested. Listen to the community feedback and offer a CJS compatible library. The route you're currently going is completely absurd and totally ignores the current landscape of the vast majority of projects using this library.

We all agree that ESM is the future. But it's exactly that, the future. The vast majority of projects are simply not there yet and what you're asking is simply not possible.

myrotvorets-team added a commit to myrotvorets/facex-transport-fetch-h2 that referenced this issue Dec 13, 2021
myrotvorets-team added a commit to myrotvorets/facex that referenced this issue Dec 13, 2021
myrotvorets-team added a commit to myrotvorets/report.myrotvorets.center that referenced this issue Dec 13, 2021
@WandersonAlves
Copy link

I have wasted 2 hours of my life checking my depedencies to see what was triggering a TypeError [ERR_UNKNOWN_FILE_EXTENSION]: Unknown file extension ".ts" when using mocha to simply resolve with a dowgrade on node-fetch. A error that i wasn't aware for...

@Ghazgkull
Copy link

Ghazgkull commented Dec 14, 2021 via email

@nannal
Copy link

nannal commented Dec 16, 2021

The two fixes everyone is going to have to default to because the dev team does not appear to listen to the users.

@btakita
Copy link

btakita commented Dec 16, 2021

The two fixes everyone is going to have to default to because the dev team does not appear to listen to the users.

All you need to do is change your dependency.

There's nothing wrong with the maintainers' decision. And they do listen to their users, just not legacy CJS projects in this case. The nodejs ecosystem is moving to ESM. Vite uses ESM & so do a growing list of other projects.

Supporting legacy CJS takes energy & resources. If staying on legacy CJS is that important to you, then the legacy CJS community is free to fork the projects that are ESM only.

@VanTanev
Copy link

VanTanev commented Dec 16, 2021

Is it possible for ESM modules to throw some sort of useful error when used in CJS context?

It seems that a big issue most people face is that their builds fail in ways that are hard to debug and understand the root issue. If it were possible for an ESM module used in CJS to fail with a very loud and explicit message, with a link to further info, that would be a great way to help people along.

@ljharb
Copy link

ljharb commented Dec 16, 2021

@VanTanev that's up to node itself; but since most ESM modules can't parse as CJS, you'll usually get a parsing error about import or export.

@NGPixel
Copy link

NGPixel commented Jan 30, 2022

Good news for everyone frustrated with the maintainers decision: fetch is finally being added to Node.js 18 🎉

@jimmywarting
Copy link
Collaborator

jimmywarting commented Jan 30, 2022

@NGPixel We don't want to support CJS and dealing with dual cjs/esm and build process hazard anymore, if you got problem with esm then then that it isn't really our fault for your build tools to not supporting ESM yet when they have had a long time to adapt to it already. TypeScript/jest/react is not the only dominant player that should decide that ESM should be halted. Someone needs to make the move and i think this will only push the development forward. if you are stuck with cjs then you can still use v2 or any other cjs wrapper that loads fetch lazily or just don't choose node-fetch anymore - it's also still very much possible to load esm-only modules in cjs

but hey this is Cool, then we can recommend ppl to start using it whenever it's available.

it got very much hype for the small amount of time that PR have been open.

I wonder what that would mean for the feature of node-fetch... deprecation? just a fallback for older NodeJS versions? an enhanced version that adds a few extra tools along with built in fetch such as cache storage, retry, ftp:// support, cookie management, debugging, intercepting request like a mitm that could respondWith(new Response(...))?

@bitinn
Copy link
Collaborator

bitinn commented Jan 31, 2022 via email

@bamapookie
Copy link

A year and a half after this issue was opened and we stepped on the rake. We have a fairly large TypeScript serverless setup running on AWS Lambda, and conversion to ESM is still a no-go. I'm glad there are other options out there now, and that we were able to find this thread showing the lack of empathy the maintainers have toward their users.

I would like to ask the maintainers: in the year and a half since v3 was released, are you now using any features that would necessitate an ESM-only build? Or have you been maintaining 2 separate codebases for little-to-no reason?

@heath-freenome
Copy link

@bamapookie Honestly, this library is no longer relevant given that node 18 now supports fetch. I imagine that the maintainers will be happy to deprecate and eventually archive this library.

@SomaticIT
Copy link
Contributor

A year and a half after this issue was opened and we stepped on the rake. We have a fairly large TypeScript serverless setup running on AWS Lambda, and conversion to ESM is still a no-go. I'm glad there are other options out there now, and that we were able to find this thread showing the lack of empathy the maintainers have toward their users.

I would like to ask the maintainers: in the year and a half since v3 was released, are you now using any features that would necessitate an ESM-only build? Or have you been maintaining 2 separate codebases for little-to-no reason?

Hello, you could use node-fetch-cjs which is a wrapper of this repo that bundle and convert this library to cjs format. It is automatically updated when a new release is published.

@jimmywarting
Copy link
Collaborator

it's true that we eventually want to deprecate & archive this as NodeJS now has fetch built in.
it's more or less our recommendation now, so not much new feature is put into our. but we strive for some backward comp for those who can't update their NodeJS version to newer version yet.

Perhaps this will be deprecated & archived when NodeJS v22 gets released and v18 becomes LTS
When v20 becomes LTS, then maybe we can depend on the undici package instead as the lowest targeted supported version is 16 (at which point they introduced web streams)

our recommended way of importing a ESM only package is by using async import from a cjs project.
or doing a simple but small wrapper of it:

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

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

No branches or pull requests