node-fetch / node-fetch

A light-weight module that brings the Fetch API to Node.js
MIT License
8.75k stars 1.03k forks source link

ESM only breaks everything #1263

Open yinzara opened 3 years ago

yinzara commented 3 years ago

Following the conversation in: https://github.com/sindresorhus/meta/discussions/15#discussioncomment-1256621

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.
herberthobregon commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

[...] , 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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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 https://github.com/nodejs/help/issues/2267

herberthobregon commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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

This makes absolutely no sense.

martinrotter commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 https://github.com/node-fetch/node-fetch/issues/1266#issuecomment-913216211

yinzara commented 3 years ago

@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.

https://github.com/microsoft/TypeScript/issues/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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

Ghazgkull commented 3 years ago

That's a really nice suggestion, Matt. Thanks for contributing to the conversation.

jimmywarting commented 3 years ago

@theLMGN ESM supports top level await...

  1. get all the paths to an array
  2. map the array over the async import() function to get a array of promises
  3. await all files (promises) to load async
  4. then export it
    export default await Promise.all(['./a.js', './b.js'].map(path => import(path)))

Also, as far as I'm aware, you can't use ESM from REPL.

"Authors can tell Node.js to treat JavaScript code as ECMAScript modules via the .mjs file extension, the package.json "type" field, or the --input-type flag"

so you can do node --input-type=module index.js without having a package.json

async import() also works in commonjs

cobaltt7 commented 3 years ago

Top level await is only available in 14.0+.

Also I didn't think you could await .map?


From: Jimmy Wärting @.> Sent: Tuesday, September 7, 2021 4:16:46 PM To: node-fetch/node-fetch @.> Cc: Paul Reid @.>; Manual @.> Subject: Re: [node-fetch/node-fetch] ESM only breaks everything (#1263)

@theLMGNhttps://github.com/theLMGN ESM supports top level await...

  1. get all the paths to an array
  2. map the array over a the async import() function
  3. await all files to load (async)
  4. export it

export default await ['./a.js', './b.js'].map(path => import(path))

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/node-fetch/node-fetch/issues/1263#issuecomment-914635314, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AOT5DEQB3XOH5QCAC5VXH5TUAZ6L5ANCNFSM5DG4QVZA.

jimmywarting commented 3 years ago

Also I didn't think you could await .map?

Updated it to Promise.all

darthtrevino commented 3 years ago

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 commented 3 years ago

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.

herberthobregon commented 3 years ago

I don't understand the drama about this 😐. j

  • just stay to version ^2
  • don't use * in your package.json
  • npm update won't update anything you spelled right.

This is like python 2 and python 3, if you wanted to stay in python 2 you used the previous versions until you decided to use version 3. Little by little everything will be updated

NGPixel commented 3 years ago

@herberthobregon What about any new user installing node-fetch because it's used in so many tutorials and other projects that have not been updated to specifically use the ^2 version? It's introducing unnecessary problems and debugging as to why it doesn't work. It also makes 2 separate versions to maintain. What guarantees that 2.x will keep being updated at the same level as the 3.x?

This is nothing like python 2 and 3, as CJS is not a deprecated feature and is still marked as STABLE and will stay that way for the foreseeable future. The fact that node-fetch is primarily used server-side but cannot be used as a CJS module makes even less sense.

Please stop suggesting this "solution" which isn't one. You clearly doesn't understand the implications of forcing users to stay on 2.x when there's no need to be. There's absolutely no other legitimate reason to propose this other than ego at this point.

mhf-ir commented 3 years ago

@herberthobregon python 2 to 3 or php 5 to 7 or 8 and etc ... have many deprecation stuff.

Is CJS deprecated in Node.JS? so simple question. Is library/framework developer must drop core stable functionality of nodejs ?

For instance in main stream of development if you're using: https://nodejs.org/dist/latest-v16.x/docs/api/net.html#net_socket_buffersize You must drop it in next version (minor/major) like php/python/golang or any libraries or framework because it's deprecated.

as semantic versioning convention, previews version(Version 2) of this library will is not main stream of development. but used in main stream of development on many application and new version is not compatible with standard CJS in node.js.

It's simple; CJS is stable/mature of Node.JS development and drop this for what ?!.

herberthobregon commented 3 years ago

In other issues, version 2 will continue to be updated. If the project is not updated is open source, you can do a PR.

CJS will eventually be deprecated. CJS is not a standard. ESM if it is.

I work my projects in version 2 with typescript and there is no problem. and I don't think fetch will drastically change your API. 🤷‍♂️ That would breaks all the browsers in the world

Fetch is a feature of ESM(browsers) not CJS. The decision to move to ESM is correct.

Use Typescript and you will never have problems, you will always write your code in esnext. When ESM is the way to write to node and CJS is deprecated, Typescript (tsc) will update to compile to ESM. Or you can use esbuild

NGPixel commented 3 years ago

@herberthobregon Wow... Now that's what we call tunnel vision.

You said it yourself: "Fetch is a feature of ESM(browsers)" except node-fetch is mainly used.... on the server. NOT browsers. 👏 Is ESM the first-party import method on browsers? Yes. Is it on the server? Absolutely not. Not even close.

Please stop spreading BS concerning CJS, which is NOT deprecated and will NOT be for the foreseeable future. See https://github.com/nodejs/node/issues/33954

Just because YOU don't have any issues with your projects doesn't mean everybody else is in the same boat.

herberthobregon commented 3 years ago

@NGPixel I hope that when it is deprecated you will return to this thread and tell me that you were wrong.

Bad practices make changes like this affect you. Stop using * in your package and life will be easier

NGPixel commented 3 years ago

Nobody is using * in their project. That's not the point. Please take the time to actually read the issues reported above which are not resolved by simply staying on the 2.x branch forever.

How can I be wrong when CJS is not planned for deprecation in any of the upcoming Node.js release? You're not making any sense... If your only response to everything is "ESM is the future", then sorry but this topic really isn't addressed to you.

yinzara commented 3 years ago

@herberthobregon There are so many problems with TypeScript and ESM that still that require "--experimental-" flags that I'm shocked you would even suggest that. https://github.com/microsoft/TypeScript/issues/43329 https://github.com/microsoft/TypeScript/pull/44501

I don't know what you're even talking about with "use TypeScript and you'll never have problems":

const fetch = await import("node-fetch")

does not work currently unless your project itself has been updated to ESM as well which is not simply something you can "switch" even if your whole codebase is TypeScript if it was written targeting CJS. You might have used "__dirname" or used "require" in a synchronous context for the lazy loading (and there are still plenty of other things that are incompatible). That would require you to completely rewrite your codebase looking for all those examples potentially across multiple repositories that will not be compatible until all have changed.

You can't even:

import something from './foo'

without either rewriting it to "./foo.js" or using "--experimental-specifier-resolution=node" when you start NodeJS again requiring a rewrite or experimental flags (not something you should be using on a production application).

Ignoring ALL of this, using Jest, the most commonly used testing framework, is still not fully supported with TypeScript and ESM even if your whole project is ESM. Mocking is still broken as is any imports without the "--experimental-specifier-resolution=node" https://github.com/facebook/jest/issues/9430

You seem to simply be ignoring everyone's concerns and just strawman echoing what you've already said and has been refuted.

ESM is not ready to be forced on the whole community, especially not from a library that is specifically intended for use in NodeJS only.

yinzara commented 3 years ago

I encourage all of those listening to this issue to comment/vote for the PR #1288 which will add CJS transpiled assets to the published package allowing CJS users to continue to use this library as they did originally. I updated the documentation to reflect that in the future, maintainers may remove this support and users are encouraged to swap to the ESM syntax as they can.

jimmywarting commented 3 years ago

You can't even:

import something from './foo'

without either rewriting it to "./foo.js" or using "--experimental-specifier-resolution=node" when you start NodeJS again requiring a rewrite or experimental flags (not something you should be using on a production application).

Using extensionless import is no good recommendation/pattern to follow. This don't work well in browser or in Deno. Deno and Browsers can not guess what file to import if it don't know the full path. Sure, node-fetch is built for NodeJS, but one of our ambition is that we also want your code to work equally as good for the browser and Deno as well.

yinzara commented 3 years ago

You can't even:

import something from './foo'

without either rewriting it to "./foo.js" or using "--experimental-specifier-resolution=node" when you start NodeJS again requiring a rewrite or experimental flags (not something you should be using on a production application).

Using extensionless import is no good recommendation/pattern to follow. This don't work well in browser or in Deno. Deno and Browsers can not guess what file to import if it don't know the full path. Sure, node-fetch is built for NodeJS, but one of our ambition is that we also want your code to work equally as good for the browser and Deno as well.

You've made my argument for me! You have to literally rewrite your TypeScript library/app to convert it to ESM. Not a single person every wrote an import in TypeScript that had a '.js' extension until they wanted to support ESM.

use TypeScript and you'll never have problems <- NOT TRUE

And your statement "What about the browser or Deno?"

This package is literally called node-fetch. It's the NodeJS implementation for the browser Fetch API. Why on earth would you argue from the point of view of the Browser or Deno? They both have the Fetch API built in!

yinzara commented 3 years ago

With 45 upvotes on this issue and 3 down votes, what will it take to convince the maintainers what they're doing is detrimental to the JS ecosystem as a whole? Every argument that's made for ESM has multiple arguments against it.

Thus far, the only real answers we get for "Why force ESM with node-fetch?"

  1. "It's the future!" - might be, but it's certainly not the present. The ecosystem can't support it yet.
  2. "It's more correct!" - might be, but is it worth causing millions of developers around the world a massive amount of work that right now won't even solve their problems with no real world benefit currently simply because you think that's the right way it should be written
  3. "There are workarounds!" - as we've shown in the above issues, there are NOT workarounds for MANY use cases
  4. "The package is smaller!" - it's 23k and it's not like that 23k is ever included in anything other than server-side apps. We aren't concerned with browser downloads of node-fetch
  5. "You need a transpilier!" - true but there wasn't a great reason to go fully ESM at this point anyway (for all the above reasons) so it's a reasonable work around while the ecosystem adjusts
  6. "Just stay on 2.x! We'll support it!" - that doesn't fix a lot of the issues raised related to automatic package updates from RenovateBot/Greenkeeper/etc and simply from npm install node-fetch breaking for most of the examples you'll find online
  7. "Use TypeScript and it'll just work!" - simply untrue for all the reasons listed above. TypeScript does not even fully support ESM yet natively

Even the "Medium" article written last year by package maintainers for the 3.x release includes examples that will break with 3.x! https://medium.com/@kepinski/node-fetch-v3-beta-is-out-9fbb8091999

jimmywarting commented 3 years ago

This package is literally called node-fetch. It's the NodeJS implementation for the browser Fetch API. Why on earth would you argue from the point of view of the Browser or Deno? They both have the Fetch API built in!

yes Browser and Deno has fetch built in but it's not our code that should be meant to work in other environments. it's all about your code. We want your code to work the same way in different environments too. And we also want to be able to import this package without npm using node's new http Import syntax so something like this works:

import fetch from 'https://cdn.xyz/node-fetch@3/mod.js'
herberthobregon commented 3 years ago

If you write in typescript from the beginning you can compile your application to CJS or ESM, That's what I meant. (Remember that there is not only tsc) (I personally think writing a project (from zero) in nodejs is bad practice. You should always write in typescript)

@yinzara Node-fetch It has more than 22 million downloads, Not only 45, Think that if you don't have a problem, you don't enter to github, here are only those who had some problem.

I don't see the problem by using version 2 if it is in CJS For this reason Version 3 is a major update Actually I use the version 2 🤷‍♂️

NGPixel commented 3 years ago

@jimmywarting Both PRs are either closed or locked. How much longer are you going to ignore the obvious that ESM only is a train wreck for this project?

So far there has been 0 legitimate argument in favor of ESM-only other than "it's the future".

This package could be universal and the PRs are proof. As @yinzara mentioned, this issue has over 45 votes, you guys really need to wake up and consider all the problems that have been pointed out so far. The "solutions" you proposed are simply not acceptable for an important library like this one. You're just alienating users for no reason at this point.

@herberthobregon These 45 votes are from maintainers of other projects that contribute to these "millions" of downloads. So this is quite a big number, especially in such a short amount of time. Please stop with these dumb arguments, it's not contributing to the discussion in any way.

jimmywarting commented 3 years ago

So far there has been 0 legitimate argument in favor of ESM-only other than "it's the future".

Not true, then you haven't followed the discussion of #1227 of all the drawbacks with dual support. It's not like we didn't consider dual support before going ESM-only and releasing it as stable, we had a test run in the beta period

We aren't concerned with browser downloads of node-fetch

size is a factor for some. #1217

herberthobregon commented 3 years ago

@NGPixel OK 45 is bigger for you, it's OK 😴 you need learn more. This should never have been a problem for anyone. For that reason npm handles versions

All my support for the creator of this library 🚀 Move on.

NGPixel commented 3 years ago

@jimmywarting The ESM-only build has even more drawbacks, so why was it forced upon everyone in the first place then? Why wasn't this made into a proposal first? You could have gained all the feedback that has been raised so far and avoided this scenario altogether.

@herberthobregon Again, you completely ignore all the criticism raised across the discussion. You support the creator, we get it. Move on.

herberthobregon commented 3 years ago

@NGPixel Search and find out , just keep in version ^2 to and move on. Learn more about the "ecosystem"

NGPixel commented 3 years ago

@herberthobregon Putting 👍 on your own posts and using multiple accounts to do so is just childish and sad... We're trying to have a constructive discussion here and repeating the same "keep version 2" argument over and over doesn't bring anything useful.

@jimmywarting If you had a beta and you still went ahead as no issues were raised, then maybe it's because nobody saw it in the first place. This very thread is proof of that.

Without resorting to a quick bundling hack, couldn't this library be refactored to be agnostic of how it is imported? Many libraries have done it so clearly it's possible...

yinzara commented 3 years ago

So far there has been 0 legitimate argument in favor of ESM-only other than "it's the future".

Not true, then you haven't followed the discussion of #1227 of all the drawbacks with dual support. It's not like we didn't consider dual support before going ESM-only and releasing it as stable, we had a test run in the beta period

We aren't concerned with browser downloads of node-fetch

size is a factor for some. #1217

I'll agree that we have not fully addressed the issues outlined in #1227

Specifically the bundle size issue that some have said is a problem.

As @LinusU stated in his comment, there are reasons that the dual bunding is undesirable.

It seems based on the Medium article published last year that the ESM move was not the original intent of the 3.x release.

Would you be wiling to entertain a PR that converts the library back to the original CJS implementation but keep all other 3x changes and forgo the bundler/transpiler? There are clearly some useful features of 3.x that would be useful for those of us that cannot yet support a ESM only module (which right now, provides no real benefits to the majority of your users).

There are certainly enough arguments here to explain why the ESM move is not ready. Simply reverting to the CJS implementation will not really change the usage of the application for ESM users but will make it possible to use it the way most people currently use it for CJS (the most common use right now of the library seeing the ESM is new).

The move to be pure ESM can then be done at a time that the ecosystem can support it fully (which it looks like might not be too far off) and properly planned and notified so that package maintainers that use node-fetch as a dependency have time to support the change.

You could still use the "await import" syntax if you have a dependency that is ESM only as you've now locked the library to NodeJS 12+ (something I do support).

herberthobregon commented 3 years ago

@NGPixel Are you joking? 😂Hahaha your comment amuses me, it was already clear to us that you do not have control of your project due to the little handling that you have in NPM and bundles. We are in 2021. Learn something new not just raw js mode

NGPixel commented 3 years ago

@herberthobregon I maintain an open source node.js / vue.js project of over 50 millions downloads and over 13,700 stars on GitHub, so yeah, thank you, I know what I'm talking about. What have you accomplished so far other than insulting people? If you're going for personal insults, at least take the time to do some research instead of digging your own hole even deeper. Like I said, childish and sad.

herberthobregon commented 3 years ago

@NGPixel Are you still here? Thank you, we already know that you are a superb person. You think you are right but you are wrong. Read the comments above, I would not like to repeat things. Move on

jimmywarting commented 3 years ago

The ESM-only build has even more drawbacks, so why was it forced upon everyone in the first place then? Why wasn't this made into a proposal first?

We first had a discussion/proposal about ESM. We normally never start on a PR if there is no issue about it first. Nobody where against the proposal and no one here in the participants or the upvoters in this issue where involved/aware in that discussion. it only came to your knowledge after we shipped it and trying it out. And 48 to 5 votes is not much, very many are also still supportive of ESM, but they don't come here giving this issue a -1 cuz this is not a problem for them

You are not forced to use v3 so stop using that as an argument, there is still ways to load v3 from cjs packages + v2 is very stable and very much alike when it comes to v3. and will continue to update v2 with bugs & security issues.

Have you asked yourself this: is there really any point for you to update to v3 if you use v2 already and working already? v3 don't bring in much new features it is very much alike in API-surface. in fact it removed some stuff and became more strict. in v3 We removed timeout option, res.textConverted(), changed to esm, removed the bundler/compiler and required full urls in req/res constructor (https://github.com/node-fetch/node-fetch/blob/main/docs/v3-UPGRADE-GUIDE.md)

esm-only, cjs-only and dual have all drawbacks (some less then others). I will not copy over all the comments from #1227 to this thread. to summarize some drawback briefly about a dual support: bundling to 0 dependencies as CJS cause duplication and interferences with instanceof if you depend on the same dependencies (this is the bigest/main cause against dual support). It increase the code size, maintainability and complexity of node-fetch itself. CJS-project can still import ESM-modules. so what is the point in having dual support then?

Using ESM-only open up the door for top level, ability to use other modules also written in ESM (i can think of 2 other esm-only dependencies we want to depend on) it also put pressure on others to start supporting ESM which many have long been waiting for. it also put pressure on NodeJS itself to start implementing fetch into core. it also standardize things you write as well to be supportive in more environment other than NodeJS

We know that ESM-only comes with drawbacks too, we have looked at it from both angels

The really only issue is mocking immutable exports and is mostly uniq to @yinzara usecase (one that can be solved in other ways, like using nock, i'm sure there are others way too, a wrapper function perhaps?)

You could still use the "await import" syntax if you have a dependency that is ESM only as you've now locked the library to NodeJS 12+ (something I do support).

that is what we have suggested all along. it don't take much to create a small wrapper module with only peer dependency being latest node-fetch and release it to npm as "node-fetch-cjs" as a package or just doing it yourself directly without having to change anything to dual support, involving esbuild, noesm or going back to using cjs in our and anyones else dependency of node-fetch. the solution is simple really. load it async with import()