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

Node16 module resolution does not exactly match node for dual ESM/CJS modules #56529

Closed
djcsdy opened this issue Nov 24, 2023 · 8 comments
Closed
Labels
External Relates to another program, environment, or user action which we cannot control.

Comments

@djcsdy
Copy link

djcsdy commented Nov 24, 2023

🔎 Search Terms

"ESM", "CJS", "ES Module", "CommonJS", "dual", "module", "Node16", "module resolution"

🕗 Version & Regression Information

  • This is the behavior in every version I tried (4.8.4, 4.9.5, 5.0.4, 5.1.6, 5.2.2, 5.3.2, git main), and I reviewed the FAQ for entries about moduleResolution, Node16

⏯ Playground Link

https://github.com/djcsdy/node-resolution

(This bug can't be reproduced on Playground because it requires a package.json, so I've provided a minimal git repository instead).

💻 Code

Consider the npm package "unknown" (https://www.npmjs.com/package/unknown).

This package exports a dual ESM/CJS module with bundled type definitions.

unknown's package.json includes:

{
    "type": "module",
    "main": "./dist/cjs/unknown.js",
    "module": "./dist/unknown.js",
    "exports": {
        ".": {
            "require": "./dist/cjs/unknown.js",
            "default": "./dist/unknown.js"
        }
    },
    "types": "./dist/unknown.d.ts"
}

This bug manifests when importing a project like unknown from a CommonJS project with

tsconfig.json:

{
    "compilerOptions": {
        "module": "node16",
        "strict": true
    }
}

index.ts:

import {hasProperty} from "unknown";
hasProperty({}, "foo");

🙁 Actual behavior

On import {hasProperty} from "unknown";, TypeScript reports:

index.ts:1:27 - 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("unknown")' call instead.

🙂 Expected behavior

TypeScript should recognise that the package "unknown" can be imported as a CommonJS module from a CommonJS context, or as an ES Module from an ESM context, and treat it appropriately in each case.

Additional information about the issue

It is possible to work around the problem by editing the "unknown" module's package.json to remove "type": "module".

This seems to indicate that TypeScript is treating the module as ESM-only because of that line.

This does not match node's behaviour. My understanding is that for node, "exports": { ".": { "require": "./dist/cjs/unknown.js", ... } } } takes precedence, and that node treats the module as CJS when imported using require, even though "type": "module" is set and the imported file has a plain ".js" extension.

djcsdy added a commit to djcsdy/node-resolution that referenced this issue Nov 24, 2023
@IllusionMH
Copy link
Contributor

Most likely problem is with single types declaration. See https://arethetypeswrong.github.io/?p=unknown%400.2.5

@fatcerberus
Copy link

Likely related to/duplicate of #50762

@djcsdy
Copy link
Author

djcsdy commented Nov 24, 2023

It looks like this problem is the inverse of #50762. I expected it to work because the "wrong" behaviour reported in #50762 also works... even though it shouldn't.

I did try moving the top-level types inside exports like this:

    "exports": {
        ".": {
            "require": "./dist/cjs/unknown.js",
            "default": "./dist/unknown.js",
            "types": "./dist/unknown.d.ts"
       }
    }

And that didn't solve the problem, which is the opposite to the incorrect behaviour reported in #50762.

I now realise the correct solution is to copy the d.ts file into the cjs directory, and specify:

  "exports": {
    ".": {
      "require": {
        "default": "./dist/cjs/unknown.js",
        "types": "./dist/cjs/unknown.d.ts"
      },
      "default": {
        "default": "./dist/unknown.js",
        "types": "./dist/unknown.d.ts"
      }
    }
  }

which does solve the problem.

So TypeScript does in fact implement this correctly, I just didn't understand how to use it (and nor does the author of unknown).

I will raise a PR with unknown to fix the problem.

As a data point, I have no idea how I would ever have figured out the correct way to implement this from the documentation, and I'm not some noob, I've been using TypeScript daily for almost as long as it has existed (at least ten years), and I'm on the first page of contributors to DefinitelyTyped. So I would consider this... underdocumented. Or maybe it is actually quite obvious and I missed it through a series of extraordinary coincidences.

Thankyou @IllusionMH for pointing me to https://arethetypeswrong.github.io/. I hadn't come across it before and it would probably be a good idea to point everyone to this tool in the TypeScript docs because it explains the problem and solution succinctly.

@MartinJohns
Copy link
Contributor

Related: #51876

@IllusionMH
Copy link
Contributor

IllusionMH commented Nov 24, 2023

I now realise the correct solution is to copy the d.ts file into the cjs directory, and specify:

With provided layout you shouldn't need to specify types at all - they should be found in relation to .js file (BTW if you insist to use types they should go before require in general case (when path differs) IIRC otherwise require would be matched first, and will break if #50762 fixed)

Also, AFAIK just copying types is not correct for CJS/ESM files as declarations for these formats are expected to be different.

@djcsdy
Copy link
Author

djcsdy commented Nov 24, 2023

@IllusionMH Noted, thank you.

I copied the types because in this specific case they are identical for CJS/ESM, I'm aware that they're unlikely to be the same in general.

Thank you for clarifying that TypeScript resolves types relative to the resolved js file in the presence of exports but absence of a types export, that's another thing that's extremely not obvious from the documentation.

@fatcerberus
Copy link

The TS devs have said often that you’re not supposed to use the same .d.ts for both ESM and CJS - this is one of the things arethetypeswrong.github.io warns you about. The correct way to do dual packages is to do type-checking and emit twice - and then package the corresponding declarations for each.

Of course that’s not really ergonomic today, and formal documentation on this use case is indeed scarce. See #54593.

djcsdy added a commit to djcsdy/unknown that referenced this issue Nov 25, 2023
Although these are identical to the type declarations
for ESM, it is necessary to generate separate type
declarations for CommonJS for compatibility with
TypeScript's Node16/NodeNext module resolution modes.

See microsoft/TypeScript#56529

See https://arethetypeswrong.github.io/?p=unknown%400.2.5

See https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/FalseESM.md
@andrewbranch andrewbranch added the External Relates to another program, environment, or user action which we cannot control. label Nov 27, 2023
@typescript-bot
Copy link
Collaborator

This issue has been marked as "External" and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 30, 2023
BTOdell pushed a commit to BTOdell/unknown that referenced this issue Apr 18, 2024
Although these are identical to the type declarations
for ESM, it is necessary to generate separate type
declarations for CommonJS for compatibility with
TypeScript's Node16/NodeNext module resolution modes.

See microsoft/TypeScript#56529

See https://arethetypeswrong.github.io/?p=unknown%400.2.5

See https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/FalseESM.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
External Relates to another program, environment, or user action which we cannot control.
Projects
None yet
Development

No branches or pull requests

6 participants