fix: load vitest configs as ESM to fix broken test suite for older versions of node#502
Open
shanefontaine wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The test suite only works on
nodeversions^20.19.0 || >=22.12.0. The Vitest configs import ESM-only tooling (notablyvite-tsconfig-paths), but CJS can'trequire()an ESM-only module onnodeversions below that range. Support for loading native ESM viarequire()landed innode@22.12.0and was backported tonode@20.19.0.This PR fixes test compatibility for older versions of
nodeand removes any ambiguity about the expected module type, following the official recommended usage by bothviteandvite-tsconfig-paths.Root Cause
The build system of this repo gracefully handles CJS and ESM only within the
srcdir (viatsup). However, the test configs are handled out-of-band byvitest(which usesvite). With.tsand notype:module,viteloads the bundled config viarequire(), which breaks with ESM only depsvite-tsconfig-paths. Using.mtsforces the ESM path.Where I Observed This
Locally running tests against this repo with any node version below
^20.19.0 || >=22.12.0.To reproduce:
Fix
Update the relevant
vitestfiles and configs to explicitly signal that these these files should be treated as ESM. This removes any remaining module choice ambiguity in the repo and fixes the broken test suite on oldernodeversions.Test plan
mainwithnodeversions20.18.3and22.11.0. Confirmed that it succeeds on main withnodeversions20.19.0,22.12.0, and24.10.0.node.pnpm test:nodesuite passes.pnpm typecheckclean.pnpm stylecheck(biome) clean.Suggested review order
vitest.*.config.mts— updated the file type to explicitly signal ESM.package.json— updated test scripts to reflect new file type change.tsconfig.json— updatedincludeproperty with the new file types. This allows thetscbuild tool to catch type issues (when runningpnpm typecheck).Notes
package.jsonto{ "engines": { "node": "^20.19.0 || >=22.12.0" } }moduleResolutionbesidesbundleris desired for i.e. stricter build controls, then this fix would need to be implemented in another way. However, a change of that nature would require a number of updates to the build tooling that would certainly handle this in stride.