fix: remove `"type": "module"` from vitest/package.json #1411

aleclarson · 2022-06-01T17:04:25Z

Projects whose tsconfig.json contains module or moduleResolution value of nodenext or node16 cannot import vitest in their *.spec.ts files without getting a TypeScript error about Vitest being incompatible with CommonJS.

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. ts(1471)

But in this case, TypeScript is reporting a false positive. Vitest compiles the *.spec.ts files, so it's not a problem that Vitest only has an ESM entry point.

Therefore, we should remove "type": "module" from vitest/package.json to appease TypeScript. As a result, we also need to use .mjs extensions for Vitest's compiled files, since they use import/export syntax.

Closes #325

Closes vitest-dev#325

netlify · 2022-06-01T17:04:30Z

✅ Deploy Preview for vitest-dev ready!

Name	Link
🔨 Latest commit	`5c22abe`
🔍 Latest deploy log	https://app.netlify.com/sites/vitest-dev/deploys/629cc3431bddbc00092afad3
😎 Deploy Preview	https://deploy-preview-1411--vitest-dev.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

antfu · 2022-06-01T22:09:18Z

I don't think this would be a long-term solution. Isn't that a config issue, or an issue of TypeScript?

aleclarson · 2022-06-01T22:28:55Z

Currrently, using nodenext or node16 is the only way to make TypeScript use pkg.exports field to resolve imports, so I wouldn't consider it a configuration bug for the user to fix.

Isn't using "type": "module" vs using .mjs extensions just a preference, or is there extra benefit to using the former?

jgoux · 2022-06-03T10:37:07Z

Hello, I think this issue is related to this one: #1417

I'm also using "module": "nodenext" in TypeScript.

In my opinion, if you want to write your package as ESM with "type": "module", you should comply with ESM requirement of writing your import/export of local files with their full path, including the .js extension. You'll produce compiled code that will be both compatible with ESM/CJS.

I'm working on a little monorepo which is ESM first with TypeScript for experiment/demo purpose and I've already encountered the same issue here: tinyhttp/tinyhttp#359

edit: Sorry I didn't realize in your case you are using "module": "nodenext" without your own module being "type": "module", I didn't realize it was possible. 😅

sheremet-va · 2022-06-04T13:00:27Z

I don't think this would be a long-term solution. Isn't that a config issue, or an issue of TypeScript?

I think so too. We had an issue that was fixed by #1417, but I think that's it? Removing type: "module" is just a workaround, TypeScript should work either way.

aleclarson · 2022-06-04T16:01:38Z

Isn't using "type": "module" vs using .mjs extensions just a preference, or is there extra benefit to using the former?

It seems these two are interchangable. Am I wrong?

aleclarson · 2022-06-04T16:12:19Z

I don't see it as TypeScript's issue. The type field of package.json defaults to commonjs, but even CJS projects should be able to use Vitest. Unfortunately, when using nodenext or node16 module resolution, there is no tsconfig setting that tells TypeScript that test files are not CJS files.

Since Vitest does support TypeScript projects that compile to CJS (as well as TS projects that compile to ESM but don't have "type": "module" in their package.json), it falls on Vitest to avoid this issue.

sheremet-va · 2022-06-04T17:59:03Z

It seems these two are interchangable. Am I wrong?

You are not wrong. Truth be told, to run Vitest we don't even need it to be .mjs, because we run it through ViteNode, but it's better to stick with the standards.

I don't see it as TypeScript's issue.

Thank you for your explanation. This is more a configure issue. You have a single tsconfig for you source code and for your tests, which is understandable. As I understand it, TypeScript thinks it will transform import to require, and because of that it gives you an error, because it thinks it will fail at runtime (but that runtime doesn't actually happen, since tests are run with ViteNode, and not ts-node and/or compiled js). I see two ways out:

Separate tsconfig for tests, which is not ideal for DX, and hurts commonjs Node projects
We apply your workaround to trick TypeScript into thinking everything is ok, which is better for the DX, because developer doesn't need to think about it

As you might have guessed I prefer the second option, since for us DX has the highest priority.

sheremet-va · 2022-06-04T18:01:57Z

I see two ways out

Maybe there is a third:

Just add require field, pointing to the same file. This will fail when importing in Node, but will work when running Vitest, which is the important part. (but this is only true for the main module part (importing from vitest))

aleclarson · 2022-06-04T18:11:30Z

3. Just add require field, pointing to the same file.

That was the first thing I tried, but it doesn't help. The "type": "module" field in Vitest's package.json is the issue.

sheremet-va · 2022-06-04T18:13:46Z

That was the first thing I tried, but it doesn't help. The "type": "module" field in Vitest's package.json is the issue.

I guess it might not work bellow TypeScript 4.7, where they added support for exports field? Which removes this option as potential fix, yeah.

aleclarson · 2022-06-04T18:13:59Z

Truth be told, to run Vitest we don't even need it to be .mjs, because we run it through ViteNode, but it's better to stick with the standards.

I tried not using .mjs and it didn't work.

This happens when I run pnpm vitest.

(node:55373) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
(Use `node --trace-warnings ...` to show where the warning was created)
/dev/vitest-nodenext/node_modules/.pnpm/vitest@0.13.1/node_modules/vitest/dist/cli.js:1
import { EventEmitter } from 'events';
^^^^^^

SyntaxError: Cannot use import statement outside a module
    at Object.compileFunction (node:vm:352:18)
    at wrapSafe (node:internal/modules/cjs/loader:1033:15)
    at Module._compile (node:internal/modules/cjs/loader:1069:27)
    at Module._extensions..js (node:internal/modules/cjs/loader:1159:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Module._load (node:internal/modules/cjs/loader:827:12)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:170:29)
    at ModuleJob.run (node:internal/modules/esm/module_job:198:25)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:409:24)

Node.js v18.0.0

aleclarson · 2022-06-04T18:15:16Z

I guess it might not work bellow TypeScript 4.7, where they added support for exports field? Which removes this option as potential fix, yeah.

It doesn't work in 4.7+ either. TypeScript detects "type": "module" and assumes the package is ESM only.

sheremet-va · 2022-06-04T18:16:02Z

Truth be told, to run Vitest we don't even need it to be .mjs, because we run it through ViteNode, but it's better to stick with the standards.

I tried not using .mjs and it didn't work.

Yeah, what I said is only true for index part, from where you import stuff in your tests. Everything else should have appropriate extension.

By the way, this is just technicalities, we probably should have appropriate extensions for all files.

sheremet-va · 2022-06-04T18:18:17Z

It doesn't work in 4.7+ either. TypeScript detects "type": "module" and assumes the package is ESM only.

Then this is a bug in TypeScript, if my assumptions about transforming require -> import is true. Because error "the specifier only resolves to an ES module" would not be true. The specifier has a require field, which means it can be imported synchronously.

aleclarson · 2022-06-04T18:39:55Z

I've opened an issue on the TypeScript repo
microsoft/TypeScript#49391

aleclarson · 2022-06-04T18:44:26Z

This is a rare enough edge case that Vitest should just work around it, instead of expecting TypeScript to support it. It's extremely rare that a package is ESM only but still allows CJS modules to import it. Of course, in reality, test files are not really CJS modules, but nonetheless, that's what TypeScript thinks. The work around isn't a big deal anyway, so the TypeScript team will probably "kick the can down the road" instead of quickly pushing a fix.

jgoux · 2022-06-04T18:56:37Z

Why is Vitest not compiling its sources to .cjs files as well? Do you want it to be pure ESM only? I think the proper fix would be to add cjs as a target compilation, fill the "require" in exports to point to it and that's it. 🤔

aleclarson · 2022-06-04T18:58:51Z

Why is Vitest not compiling its sources to .cjs files as well? Do you want it to be pure ESM only? I think the proper fix would be to add cjs as a target compilation, fill the "require" in exports to point to it and that's it. 🤔

It reduces installation size/speed. Unless we wanted to support older versions of Node, there's no reason to publish CJS files. And if we did do that, we would have to remove "type": "module" anyway.

antfu · 2022-06-04T21:27:46Z

Can you set up a minimal reproduction so I can try different solutions? Thanks

aleclarson · 2022-06-04T21:34:34Z

@antfu https://github.com/aleclarson/repro/tree/vitest-nodenext

git clone https://github.com/aleclarson/repro -b vitest-nodenext
pnpm i

antfu · 2022-06-04T21:43:55Z

That's a weird situation. Ok I am fine with it and let's have it in the next minor as a breaking change. Can you also help to update other packages in the monorepo (namely vite-node) to use the same convention for consistentency? Thanks.

bluwy · 2022-06-06T14:28:12Z

Just noticed this issue, and I think this is a configuration issue than a Vitest/Typescript issue. According to the ts docs, .ts files would be transpiled as .js, and since the package.json defaults to "type": "commonjs" it's treated as CJS instead, hence TypeScript warns about the import.

A solution would be to use "type": "module" for the repro project, or use the .mts extension. So I think this PR should be reverted.

PS: I'm not sure why this PR fixes the issue, I think it should still error as before, maybe a bug in TypeScript.

sheremet-va · 2022-06-06T14:32:36Z

A solution would be to use "type": "module" for the repro project, or use the .mts extension. So I think this PR should be reverted.

The issue is that for some people it is not possible to move their codebase to ESM, but tests still work, so we are going with the better DX than better configuration.

I'm not sure why this PR fixes the issue, I think it should still error as before, maybe a bug in TypeScript.

I too think it's a bug, I think it should've read an exports field and see its ESM, which mean require("vitest") will fail (so, need to raise an error). But getting something from the TypeScript team is not a fast task, so I think we should stay with the solution for now.

bluwy · 2022-06-06T14:45:31Z

The solution for now won't last though if TypeScript eventually fixes the issue. My thought was that TypeScript should still error out even after this PR's changes (currently it doesn't error, which feels like a bug).

IIUC the config workaround is only needed for nodenext or node16, which is a very new flag, and if someone is willing to go there, they should also be ready to be compliant with how TS evaluates esm/cjs. And the quick solution is to probably use .mts.

I think it's a ticking time-bomb though if we keep this change. The DX will bite back in the future.

aleclarson · 2022-06-09T21:15:55Z

Looks like this can be reverted. There's a better solution that avoids the time-bomb issue that @bluwy mentioned.

See microsoft/TypeScript#49391 (comment)

Co-authored-by: Vladimir Sheremet <sleuths.slews0s@icloud.com>

fix: remove "type": "module" from vitest/package.json

6f0ab29

Closes vitest-dev#325

aleclarson mentioned this pull request Jun 4, 2022

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

Closed

antfu added the breaking change label Jun 4, 2022

Merge branch 'main' into fix/325

5c22abe

antfu merged commit b4a9b0b into vitest-dev:main Jun 5, 2022

bluwy mentioned this pull request Jun 6, 2022

fix: correct export extension #1439

Merged

bluwy mentioned this pull request Jun 11, 2022

fix!: use type module (revert #1411) #1465

Merged

sheremet-va added a commit that referenced this pull request Oct 7, 2022

fix!: use type module (revert #1411) (#1465)

652f160

Co-authored-by: Vladimir Sheremet <sleuths.slews0s@icloud.com>

e-chan1007 mentioned this pull request Jan 19, 2023

@nuxt/test-utils-edge doesn't work with vitest v0.24.0 or later nuxt/nuxt#15404

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: remove `"type": "module"` from vitest/package.json #1411

fix: remove `"type": "module"` from vitest/package.json #1411

aleclarson commented Jun 1, 2022 •

edited

Loading

netlify bot commented Jun 1, 2022 •

edited

Loading

antfu commented Jun 1, 2022 •

edited

Loading

aleclarson commented Jun 1, 2022 •

edited

Loading

jgoux commented Jun 3, 2022 •

edited

Loading

sheremet-va commented Jun 4, 2022

aleclarson commented Jun 4, 2022

aleclarson commented Jun 4, 2022

sheremet-va commented Jun 4, 2022

sheremet-va commented Jun 4, 2022 •

edited

Loading

aleclarson commented Jun 4, 2022

sheremet-va commented Jun 4, 2022

aleclarson commented Jun 4, 2022

aleclarson commented Jun 4, 2022

sheremet-va commented Jun 4, 2022 •

edited

Loading

sheremet-va commented Jun 4, 2022

aleclarson commented Jun 4, 2022

aleclarson commented Jun 4, 2022 •

edited

Loading

jgoux commented Jun 4, 2022 •

edited

Loading

aleclarson commented Jun 4, 2022

antfu commented Jun 4, 2022

aleclarson commented Jun 4, 2022

antfu commented Jun 4, 2022

bluwy commented Jun 6, 2022

sheremet-va commented Jun 6, 2022

bluwy commented Jun 6, 2022

aleclarson commented Jun 9, 2022

fix: remove "type": "module" from vitest/package.json #1411

fix: remove "type": "module" from vitest/package.json #1411

Conversation

aleclarson commented Jun 1, 2022 • edited Loading

netlify bot commented Jun 1, 2022 • edited Loading

✅ Deploy Preview for vitest-dev ready!

antfu commented Jun 1, 2022 • edited Loading

aleclarson commented Jun 1, 2022 • edited Loading

jgoux commented Jun 3, 2022 • edited Loading

sheremet-va commented Jun 4, 2022

aleclarson commented Jun 4, 2022

aleclarson commented Jun 4, 2022

sheremet-va commented Jun 4, 2022

sheremet-va commented Jun 4, 2022 • edited Loading

aleclarson commented Jun 4, 2022

sheremet-va commented Jun 4, 2022

aleclarson commented Jun 4, 2022

aleclarson commented Jun 4, 2022

sheremet-va commented Jun 4, 2022 • edited Loading

sheremet-va commented Jun 4, 2022

aleclarson commented Jun 4, 2022

aleclarson commented Jun 4, 2022 • edited Loading

jgoux commented Jun 4, 2022 • edited Loading

aleclarson commented Jun 4, 2022

antfu commented Jun 4, 2022

aleclarson commented Jun 4, 2022

antfu commented Jun 4, 2022

bluwy commented Jun 6, 2022

sheremet-va commented Jun 6, 2022

bluwy commented Jun 6, 2022

aleclarson commented Jun 9, 2022

fix: remove `"type": "module"` from vitest/package.json #1411

fix: remove `"type": "module"` from vitest/package.json #1411

aleclarson commented Jun 1, 2022 •

edited

Loading

netlify bot commented Jun 1, 2022 •

edited

Loading

antfu commented Jun 1, 2022 •

edited

Loading

aleclarson commented Jun 1, 2022 •

edited

Loading

jgoux commented Jun 3, 2022 •

edited

Loading

sheremet-va commented Jun 4, 2022 •

edited

Loading

sheremet-va commented Jun 4, 2022 •

edited

Loading

aleclarson commented Jun 4, 2022 •

edited

Loading

jgoux commented Jun 4, 2022 •

edited

Loading