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

"require" condition not respected when imported package has "type": "module" #49391

Closed
aleclarson opened this issue Jun 4, 2022 · 7 comments
Closed
Assignees
Labels
External Relates to another program, environment, or user action which we cannot control.

Comments

@aleclarson
Copy link

Bug Report

  1. The vitest package uses "type": "module"
  2. My project is using "moduleResolution": "node16" in its tsconfig
  3. My project has no "type" field in package.json, so it defaults to commonjs

Currently, Vitest doesn't have a require condition in its exports field, but even if it did, TypeScript 4.7 still assumes the package is ESM only. This leads to TypeScript thinking that vitest cannot be imported by test files, as the test files are assumed to be CJS modules, but Vitest actually compiles them as ESM.

I have a PR open (vitest-dev/vitest#1411) that removes "type": "module" and uses .mjs extensions instead, and this avoids the issue. But the Vitest team thinks this is something that TypeScript should fix. I believe this is an edge case and so Vitest should fix it, but I'm writing this issue to get the TypeScript team's opinion on the matter.

🔎 Search Terms

  • require condition
  • type module
  • node16 moduleResolution
  • nodenext

🙁 Actual behavior

When a test file imports vitest, it gets this error:

Module 'vitest' cannot be imported using this construct. The specifier only resolves to an ES module, 
which cannot be imported synchronously. Use dynamic import instead.

Even if I add a require condition the exports map of Vitest.

🙂 Expected behavior

Vitest should be able to use a require condition to signal to TypeScript that CJS modules are allowed to import the vitest package without receiving an error message.

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Jun 9, 2022
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 4.8.1 milestone Jun 9, 2022
@weswigham
Copy link
Member

If vitest compiles its esm to cjs at runtime (build time?) and the imports will successfully resolve to cjs format files, then it's presenting a cjs api at runtime and its types should reflect that. Removing type: module and using .mjs extensions instead won't change anything - the files will still be esm format either way. If you want imports from cjs consumers to always succeed, you need to provide cjs entry points - either declaration files (or source) in a type: commonjs context or .d.cts files. I assume vitest is only used via bundler (likely vite)? Because you'd have encountered issues in node itself long ago otherwise. module: nodenext doesn't model whatever bespoke resolution vite does; it models node itself. If there's a mismatch there, you'd have to inform typescript of the equivalences via explicit type definitions.

@weswigham
Copy link
Member

weswigham commented Jun 9, 2022

In short, your idea of using a require condition to point TS is the kernel of the right way to go, but you also need the file you're pointing the condition at to actually be the right format - the condition alone doesn't imply the format. If you only wanna solve for TS (and not non-vite node callers), supplying a combination require + types condition pointing at a .d.cts file (or .d.ts in type: commonjs) is the way to go.

@aleclarson
Copy link
Author

Removing type: module and using .mjs extensions instead won't change anything

Correction: It removes the TypeScript warning mentioned in the "Actual behavior" section.

@aleclarson
Copy link
Author

aleclarson commented Jun 9, 2022

If you only wanna solve for TS (and not non-vite node callers), supplying a combination require + types condition pointing at a .d.cts file (or .d.ts in type: commonjs) is the way to go.

I can confirm your solution also removed the TypeScript warning.

".": {
  "require": {
    "types": "./dist/index.d.cts",
    "default": "./dist/index.cjs"
  },
  "types": "./dist/index.d.ts",
  "import": "./dist/index.js"
},

I had tried only a require condition before, but not a combination of require/default and require/types conditions.

@weswigham weswigham added External Relates to another program, environment, or user action which we cannot control. and removed Needs Investigation This issue needs a team member to investigate its status. labels Jun 13, 2022
@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.

@tilgovi

This comment was marked as resolved.

@tilgovi
Copy link

tilgovi commented Jul 19, 2022

It seems I was wrong. I tried to reproduce the minimal example here and I could not. It may be that I had an additional types condition pointing at the ESM and that is what had led TypeScript to consider the ESM despite the presence of require. Pardon the noise.

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

5 participants