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

TypeScript errors with moduleResolution nodenext #125

Open
rjwalters opened this issue Jul 22, 2023 · 16 comments
Open

TypeScript errors with moduleResolution nodenext #125

rjwalters opened this issue Jul 22, 2023 · 16 comments

Comments

@rjwalters
Copy link

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"]
}

stephenh/ts-proto#891

@stephenh
Copy link

stephenh commented Jul 22, 2023

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
Copy link

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.

@rjwalters rjwalters changed the title typescript error when "skipLibCheck=false" TypeScript errors with moduleResolution nodenext Jul 22, 2023
@fs-eire
Copy link

fs-eire commented Jul 24, 2023

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

@stephenh
Copy link

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

@Silventino
Copy link

The PR from @fs-eire works for me.

@tzvc
Copy link

tzvc commented Aug 24, 2023

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

@timeimp
Copy link

timeimp commented Sep 1, 2023

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?

@dcodeIO
Copy link
Owner

dcodeIO commented Sep 1, 2023

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

@fs-eire
Copy link

fs-eire commented Sep 1, 2023

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

@timeimp
Copy link

timeimp commented Sep 8, 2023

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?

@jamie-pate
Copy link

jamie-pate commented Nov 1, 2023

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

@ljrahn
Copy link

ljrahn commented Nov 6, 2023

Is this getting resolved soon??

@michalgrzyska
Copy link

It affects Firebase Functions in some cases 💀

lionello added a commit to DefangLabs/pulumi-defang that referenced this issue Nov 10, 2023
lionello added a commit to DefangLabs/pulumi-defang that referenced this issue Dec 1, 2023
lionello added a commit to DefangLabs/pulumi-defang that referenced this issue Dec 1, 2023
lionello added a commit to DefangLabs/pulumi-defang that referenced this issue Dec 3, 2023
lionello added a commit to DefangLabs/pulumi-defang that referenced this issue Dec 3, 2023
lionello added a commit to DefangLabs/pulumi-defang that referenced this issue Dec 4, 2023
lionello added a commit to DefangLabs/pulumi-defang that referenced this issue Dec 4, 2023
lionello added a commit to DefangLabs/pulumi-defang that referenced this issue Dec 4, 2023
@ceolinrenato
Copy link

Any news on this issue?

@fs-eire
Copy link

fs-eire commented Jan 18, 2024

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')\"",
    ...

@ceolinrenato
Copy link

ceolinrenato commented Jan 18, 2024

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

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

No branches or pull requests