long.js icon indicating copy to clipboard operation
long.js copied to clipboard

TypeScript errors with moduleResolution nodenext

Open rjwalters opened this issue 2 years ago • 16 comments

I am having trouble upgrading a package that recently switched a dependency from long@^4 to long@^5:

node_modules/long/umd/index.d.ts:1:18 - error TS1479: The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("../index.js")' call instead.

1 import Long from "../index.js";


Found 1 error(s).

I can suppress the error by skipping library checks in my tsconfig.json file but perhaps this points to a problem with UMD?

Here is my complete tsconfig for reference (with skipLibCheck=false):

{
  "compilerOptions": {
    "module": "CommonJS",
    "resolveJsonModule": true,
    "declaration": false,
    "declarationMap": false,
    "removeComments": true,
    "emitDecoratorMetadata": true,
    "experimentalDecorators": true,
    "target": "es6",
    "lib": ["es6", "dom"],
    "sourceMap": true,
    "allowSyntheticDefaultImports": true,
    "outDir": "./dist",
    "baseUrl": "./",
    "moduleResolution": "nodenext",
    "esModuleInterop": true,
    "incremental": false,
    "skipLibCheck": false,
    "noEmit": false,
    "strictNullChecks": true,
    "noImplicitAny": true,
    "strictBindCallApply": true,
    "forceConsistentCasingInFileNames": true,
    "noFallthroughCasesInSwitch": true,
    "allowJs": true,
    "strict": false,
    "types": ["mocha", "chai", "node"]
  },
  "include": ["src/**/*", "hardhat.config.cjs"]
}

https://github.com/stephenh/ts-proto/issues/891

rjwalters avatar Jul 22 '23 21:07 rjwalters

Coming from the ts-proto issue, a minimal reproduction is this index.ts:

import Long = require("long")
console.log("Hello", Long);

Works with this minimal tsconfig (tsc compiles cleanly):

{
  "compilerOptions": {
    "lib": ["ESNext", "dom"],
    "module": "commonjs",
    "skipLibCheck": false,
    "target": "esnext",
    "strict": true
  }
}

But breaks when moduleResolution: nodenext is added:

{
  "compilerOptions": {
    "lib": ["ESNext", "dom"],
    "module": "commonjs",
    "moduleResolution": "nodenext",
    "skipLibCheck": false,
    "target": "esnext",
    "strict": true
  }
}

My guess is that @rjwalters has some dependencies in his node_modules/ using the older import Long = require("long") syntax, which seems to force TypeScript into "importing the UMD module" (which seems reasonable), but then with his project's moduleResolution: nodenext turned on, it seems to realize/think that the umd/index.d.ts importing ../index.js/d.ts is invalid.

...ah, yes, here it is:

"Finally, it’s worth noting that the only way to import ESM files from a CJS module is using dynamic import() calls. This can present challenges, but is the behavior in Node.js today."

https://www.typescriptlang.org/docs/handbook/esm-node.html

Fwiw I think the umd/index.d.ts should just be a copy/paste of index.d.ts (with the = change), similar to ./index.js and ./umd/index.js fundamentally being separate files, and that will probably resolve this issue.

As-is, we've got two totally separate JS files (umd/esm), but are trying to reuse the types across the two, and TS's stricter moduleResolution: nodenext setting is realizing that's not supported.

Granted, @rjwalters can a) use skipLibCheck: true to have tsc not bother double-checking long.js's types against his own stricter tsconfig settings, or b) not use moduleResolution: nodenext, but both of those seem somewhat reasonable.

stephenh avatar Jul 22 '23 22:07 stephenh

Fwiw @rjwalters you might consider re-titling this issue (if you're allowed to as the reporter) to "TypeScript errors with moduleResolution nodenext" b/c I believe that is the core issue here.

Granted, I think it does take a library using import Long = require("..."), but in long v4/CommonJS that was the most kosher way to import a directly-exported const.

stephenh avatar Jul 22 '23 22:07 stephenh

I met similar error before, and #124 fixed this for me. Not sure if it works for you

fs-eire avatar Jul 24 '23 23:07 fs-eire

Thanks @fs-eire ! I think that's exactly what we need. Thanks for creating the PR!

stephenh avatar Jul 25 '23 00:07 stephenh

The PR from @fs-eire works for me.

Silventino avatar Aug 21 '23 02:08 Silventino

Running into the same issue. Anyone has an idea as to why this is breaking only now? I've been running this code for month wihtout isues

tzvc avatar Aug 24 '23 07:08 tzvc

Typescript 5.2 has been released and you can no longer mix-and-match.

I upgraded my repo to Typescript 5.2, Node 18 etc... and got the same error as @rjwalters

The fix in #124 works but it probably not going to be a solution in production environments.

Is there a path to fixing this that I could look at @dcodeIO or is this a pretty big change for the repo overall?

timeimp avatar Sep 01 '23 07:09 timeimp

Perhaps if the UMD typings are generated by build / excluded from Git, with that one line automatically amended, instead of manually duplicating these?

dcodeIO avatar Sep 01 '23 08:09 dcodeIO

Perhaps if the UMD typings are generated by build / excluded from Git, with that one line automatically amended, instead of manually duplicating these?

Yes. The recommended way is to copy the file in npm prepack script. We don't need the duplicated index.d.ts file in the code base. We only need it in the NPM package

fs-eire avatar Sep 01 '23 18:09 fs-eire

Perhaps if the UMD typings are generated by build / excluded from Git, with that one line automatically amended, instead of manually duplicating these?

I guess so.

What would be the problems if this solution was put in place?

I'd love to open a PR with some kind of fix, but I'm not sure of the problems that might happen if UMD typings are excluded?

timeimp avatar Sep 08 '23 06:09 timeimp

This also affects node16 (which is currently an alias of nodenext) which has strong language about not using any other setting with node:

Because node16 and nodenext are the only module options that reflect the complexities of Node.js’s dual module system, they are the only correct module options for all apps and libraries that are intended to run in Node.js v12 or later, whether they use ES modules or not.

Also, skipLibCheck sucks and I don't like using it if I can avoid it :D

jamie-pate avatar Nov 01 '23 23:11 jamie-pate

Is this getting resolved soon??

ljrahn avatar Nov 06 '23 17:11 ljrahn

It affects Firebase Functions in some cases 💀

michalgrzyska avatar Nov 07 '23 14:11 michalgrzyska

Any news on this issue?

ceolinrenato avatar Jan 18 '24 01:01 ceolinrenato

Somehow the codebase owner is unwilling to merge my PR #124. I am currently using the following workaround in my package.json to make typescript happy:

  "scripts": {
    ...
    "preprepare": "node -e \"require('node:fs').copyFileSync('./node_modules/long/index.d.ts', './node_modules/long/umd/index.d.ts')\"",
    ...

fs-eire avatar Jan 18 '24 01:01 fs-eire

Somehow the codebase owner is unwilling to merge my PR #124. I am currently using the following workaround in my package.json to make typescript happy:

  "scripts": {
    ...
    "preprepare": "node -e \"require('node:fs').copyFileSync('./node_modules/long/index.d.ts', './node_modules/long/umd/index.d.ts')\"",
    ...

Thanks for providing this workaround. Hopefully the PR get's revised/merged, it's been months. protobufjs also has this package as a dependency so it affects lots of projects

ceolinrenato avatar Jan 18 '24 01:01 ceolinrenato