Vitest incorrectly injects __dirname when type is module

[snyamathi]

Describe the bug

I'm in the process of migrating a large codebase from CJS to ESM. One of the main "gotchas" is
that there is No __filename or __dirname.

I'm getting a lot of tests which incorrectly pass when they should actually fail because Vitest seems to inject the __dirname into the ES Module. This would be fine for CommonJS but is not correct for ESM.

Please see the minimal example enclosed where npx vitest run will pass, but node index.js will fail.

It actually looks like all of the CJS Module Scope is available in ESM. This is not correct and will cause a lot of issues for developers who want to accurately test against an ESM environment.

Reproduction

https://stackblitz.com/edit/vitest-dev-vitest-ixfqyt?file=index.js

$ cat index.js
export default __dirname;

$ jq '.type' package.json 
"module"

$ npx vitest run
 RUN  v0.28.4 /home/projects/vitest-dev-vitest-ixfqyt

 ✓ index.test.js (1)

 Test Files  1 passed (1)
      Tests  1 passed (1)
   Start at  09:37:40
   Duration  4.72s (transform 136ms, setup 0ms, collect 21ms, tests 3ms)

$ node index.js
Error: __dirname is not defined
    at file:///home/projects/vitest-dev-vitest-ixfqyt/index.js
    at async ESMLoader.import (https://vitestdevvitestixfqyt-psy2.w-credentialless.staticblitz.com/blitz.2913a9f2fedb71678a44d5a6b5e0b08a6f00d17f.js:6:1209048)
    at async i.loadESM (https://vitestdevvitestixfqyt-psy2.w-credentialless.staticblitz.com/blitz.2913a9f2fedb71678a44d5a6b5e0b08a6f00d17f.js:6:246622)
    at async handleMainPromise (https://vitestdevvitestixfqyt-psy2.w-credentialless.staticblitz.com/blitz.2913a9f2fedb71678a44d5a6b5e0b08a6f00d17f.js:6:989055)

System Info

System:
    OS: Linux 5.0 undefined
    CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 0 Bytes / 0 Bytes
    Shell: 1.0 - /bin/jsh
  Binaries:
    Node: 16.14.2 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 7.17.0 - /usr/local/bin/npm
  npmPackages:
    vitest: * => 0.28.4

Used Package Manager

npm

Validations

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
• The provided reproduction is a minimal reproducible example of the bug.

[snyamathi]

This seems to be coming from

vitest/packages/vite-node/src/client.ts

Lines 370 to 397 in ab5f892

	// Be careful when changing this
	// changing context will change amount of code added on line :114 (vm.runInThisContext)
	// this messes up sourcemaps for coverage
	// adjust `offset` variable in packages/coverage-c8/src/provider.ts#86 if you do change this
	const context = this.prepareContext({
	// esm transformed by Vite
	__vite_ssr_import__: request,
	__vite_ssr_dynamic_import__: request,
	__vite_ssr_exports__: exports,
	__vite_ssr_exportAll__: (obj: any) => exportAll(exports, obj),
	__vite_ssr_import_meta__: meta,
	// cjs compact
	require: createRequire(href),
	exports: cjsExports,
	module: moduleProxy,
	__filename,
	__dirname: dirname(__filename),
	})
	debugExecute(__filename)
	// remove shebang
	if (transformed[0] === '#')
	transformed = transformed.replace(/^\#\!.*/, s => ' '.repeat(s.length))
	// add 'use strict' since ESM enables it by default
	const codeDefinition = `'use strict';async (${Object.keys(context).join(',')})=>{{`

which is where the CJS scope variables are being provided to our ES Module.

[sheremet-va]

Yes, this is how vite-node works. We need this because frontend ecosystem is not ready for full ESM. Vitest also allows incorrect imports for the same reason.

This is very bad for testing native Node.js code, but this is what bundlers expect to work (so, users).

If you rely on this, I can only suggest using another test runner. We can probably add some kind of an option, but it would increase the complexity and basically copy paste Node.js resolution mechanism (why do that, if you can just use native Node test runner?).

[snyamathi]

Thanks for the response; I was hoping to migrate from to Vitest from Jest because Jest can't handle ES Modules (issue).

The project I'm testing is purely server side.

Our existing Jest code uses mocks in various places so it was nice to use Vitest as a way to retain our existing mocks - something which I don't know how to do with the native Node test runner. Maybe that's possible using the loader API though I haven't even begun to look at that.

I might see if I can extend the runner / executor and solve this in userland. I'd hate to rely solely on a linter, though that's another option.

[snyamathi]

@sheremet-va I did some work to determine when a module is ESM and remove the CJS scope variables in this branch: main...snyamathi:cjsCompat

However, if the expectation is that Vitest is not meant for native Node.js code then I don't know that we would really want this. I know that Vite supports SSR, but it seems like even that maintains this hybrid ESM/CJS shim as valid code.

Let me know what you think - it would be nice to have a strict ESM mode where the code must be valid without the CJS Shim assumed to be present.

[sheremet-va]

However, if the expectation is that Vitest is not meant for native Node.js code then I don't know that we would really want this. I know that Vite supports SSR, but it seems like even that maintains this hybrid ESM/CJS shim as valid code.

The main idea is to be compatible with how bundlers think ESM should work, which is not how it actually works.

Let me know what you think - it would be nice to have a strict ESM mode where the code must be valid without the CJS Shim assumed to be present.

It would be nice to have strict mode, but it involves more than removing shims. Path should have correct extensions, "default" from commonjs should not be interoped (don't allow named keys). Then there is also typescript that has a different ESM implementation and also hides these errors, and needs to be taken into account.

The other thing that will break is C8 integration, because it depends on how many keys we pass to VM wrapper.

[snyamathi]

Understood, thanks for the explanation @sheremet-va

It 100% makes sense that vitest would be primarily concerned with the needs of Vite and not meant as a general purpose test runner. We'll have to re-think our testing strategy for our ESM migration but that's a separate issue.

I'll say though that otherwise my experience with Vitest has all been great, good work you and the other contributors.

[sheremet-va]

It 100% makes sense that vitest would be primarily concerned with the needs of Vite and not meant as a general purpose test runner.

Just to reiterate, I am not against implementing ESM strict mode in Vitest (I actually think it’s a good idea), but this is not the priority right now, but definitely lives in my head as a possible direction for Vitest in the future.

[sheremet-va]

We decided to add experimental support for this, but it will take some time.

[H0R5E]

I am developing a couple of ESM + node.js libraries (for my sins) and this got me too. I'm not going to use a bundler on these libraries and I assumed this was a valid use case for vitest (which is a great tool, by the way).

If ESM is the future for node.js, then I think being not being able to catch this kind of error is going to discourage users, IMO.