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

[3.0.0-beta.10] Can't import node-fetch when using TypeScript in CJS mode #1226

Closed
SomaticIT opened this issue Jul 28, 2021 · 11 comments
Closed

Comments

@SomaticIT
Copy link
Contributor

When upgrading node-fetch to v3.0.0-beta.10 in a TypeScript project which compile modules in CommonJS mode, the following error occured:

Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: /tmp/node-fetch-test/node_modules/node-fetch/src/index.js
require() of ES modules is not supported.
require() of /tmp/node-fetch-test/node_modules/node-fetch/src/index.js from /tmp/node-fetch-test/index.js is an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which defines all .js files in that package scope as ES modules.
Instead rename /tmp/node-fetch-test/node_modules/node-fetch/src/index.js to end in .cjs, change the requiring code to use import(), or remove "type": "module" from /node-fetch-test/node_modules/node-fetch/package.json.

Reproduction

Steps to reproduce the behavior:

  1. Create an empty project:
npm init
npm i -D typescript
npm i node-fetch@3.0.0-beta.10
npx tsc --init
  1. Create a file index.ts with the following code:
import fetch from "node-fetch";

(async () => {
    const res = await fetch("https://api.github.com/repos/touchifyapp/player/releases/latest", {
        headers: { Accept: "application/vnd.github.v3+json" }
    });

    console.log(await res.json());
})();
  1. Run the project:
tsc -p .
node index.js

Expected behavior

It should be able to import node-fetch correctly.

Many projects are using commonjs. When using TypeScript in cjs mode, it is not possible to mix require and import statements. Even in traditional javascript projects, importing an ES module is a mess. you have to use the async import() syntax, you can't import the module as usual.

I think we should provide both ESM and CJS versions of the package to ensure all users can migrate to node-fetch v3.
We can use package.json exports configuration to ensure projects are using the appropriate version of the module depending on their configuration.

The simplest and quickest method could be to use esbuild which can transpile esm modules to cjs.

Your Environment

software version
node-fetch 3.0.0-beta.10
node 14.15.3
npm 7.19.1
Operating System WSL 2
typescript 4.3.5
@LinusU
Copy link
Member

LinusU commented Jul 29, 2021

(I gave a longer answer on my opinion on the change in the PR)

Since you are using Node.js 14.15.3, can't you switch from "module": "CommonJS" to "module": "ES2020" in your tsconfig.json? Then also add "type": "module" to your package.json, add the .js file extension to your imports, and you should be good to go 🚀

@hjdhjd
Copy link

hjdhjd commented Aug 3, 2021

@LinusU I have similar issues - my code happens to be a plugin for another project that itself doesn't currently support ESM yet. This represents a significant breaking change for that use case. I understand why you are pushing folks to update, but it's simply not possible in a non-trivial percentage of use cases where one is dependent on upstream packages / solutions also updating, and our user communities get impacted as a result.

My current "fix" for this is to pin the dependency to beta9 or below. Not thrilled about it, but updating to ES2020/ESM modules is just not an option that is meaningfully available to me right now since it would break the entire module/plugin, although certainly open to suggestions about how to solve for it if there's a workaround. I'd much rather keep up with the most recent versions, but don't see a good way to do it without breakage unless node-fetch offers broader compatibility with legacy ways of doing things, for the time being.

My thoughts for what it's worth - a breaking change like this is a year too soon. There's still an awful lot of code that falls into the above categories, and it isn't going to get updated overnight.

@LinusU
Copy link
Member

LinusU commented Aug 3, 2021

My thoughts for what it's worth - a breaking change like this is a year too soon. There's still an awful lot of code that falls into the above categories, and it isn't going to get updated overnight.

I personally do not think it's too soon, since all currently supported versions of Node.js supports ESM. If someone is using an older version of Node.js then they aren't getting any security patches at all at this point. I don't see why we have to keep supporting those older runtimes in the beta version of our next major release.

My current "fix" for this is to pin the dependency to beta9 or below. Not thrilled about it, but updating to ES2020/ESM modules is just not an option that is meaningfully available to me right now since it would break the entire module/plugin, although certainly open to suggestions about how to solve for it if there's a workaround.

Without knowing which system your plugin is for is hard to give a good recommendation. My general take for libraries is that they should release a new major version that drops support for older versions of Node.js, and switches from CommonJS to ESM.

There are also other possible ways forward:

You could use version 2.x of node-fetch. Is there a specific reason why you are using the 3.x beta?

You could use the following shim that uses the ESM module from a CommonJS context:

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

I'd much rather keep up with the most recent versions, but don't see a good way to do it without breakage unless node-fetch offers broader compatibility with legacy ways of doing things, for the time being.

It's always a trade off between keeping up with the most recent versions and supporting legacy workflows. I personally think the best way forward is to push on with every library to help the ecosystem forward. Many packages are moving to ESM now so most likely you'll run into another one even if we revert the changes here.

As long as these changes are made in a major release, I don't see a problem since it won't break anything for anyones current workflow. They just cannot use the latest version until the upgrade...

@hjdhjd
Copy link

hjdhjd commented Aug 3, 2021

Mostly I needed #715. 😄 But using a few of the other features / evolutions built into 3.0.

To be honest - there's nothing you wrote that I wouldn't feel the same way about in your position. I think you're right that it's beta and a major release and therefore expecting breaking changes is not unreasonable. But it does make me work more right now while I try to support my own codebase. 😄 I knew what I was getting into when I shifted to 3.0...it's on me to adapt, but also try to nudge where I can to get a bit more breathing room!

The platform in question I'm supporting is Homebridge. There's an open PR to support ESM, but it'll be a while before it gets pulled into the mainline...and that's what's creating more hand-wringing for me.

I'll try the shim you suggested out and see if I can make it work.

@Richienb
Copy link
Member

Richienb commented Aug 5, 2021

Dropping CommonJS is an intended consequnce of #1141. Since ESM imports in Node.js also support CJS, the change should come with no repurcussions. Besides, as new packages are published with the intention of only supporting ESM, the CommonJS version becomes less and less useful.

For information about migrating your package to ESM, see https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c

@image72
Copy link

image72 commented Aug 5, 2021

@Richienb nodejs packages is not only used in node version >= 12.
please give us low version nodejs user a choices.

@LinusU
Copy link
Member

LinusU commented Aug 5, 2021

@image72 may I ask why are using an older version of Node.js? Anything below version 12 is no longer receiving security updates and are end of life.

please give us low version nodejs user a choices.

Is sticking with the 2.x version of node-fetch a good enough choice? We have made other changes than just ESM which will break in Node.js versions below 12...

@image72
Copy link

image72 commented Aug 5, 2021

company have many huge online base API services, should upgrade runtime versions every 2year.
in nodejs version 12,
need Add the --experimental-modules flag when running Node.js

but, provide commonjs build is easy work. deprecate old nodejs v12 below just TSC duty

@LinusU
Copy link
Member

LinusU commented Aug 5, 2021

should upgrade runtime versions every 2year.

Node.js 12 was released over two years ago 🤔

need Add the --experimental-modules flag when running Node.js

This is no longer required, as long as you are running Node.js 12.17.0 or newer...

but, provide commonjs build is easy work.

There is actually some pitfalls, drawbacks, and just additional work with doing that. So we would prefer to stick with ESM...

@gbiryukov

This comment has been minimized.

@jimmywarting
Copy link
Collaborator

Locking b/c it's nicer to keep the discussion in one place.
(We still keep a open mind and looking for a solution - it dose not mean we will overlook this Issue)

Feel free to keep the discussion in #1227 where most of the discussion is already happening

@node-fetch node-fetch locked and limited conversation to collaborators Aug 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants