Repository navigation
fix: yargs esm fix test - #8460
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a .pnpmfile.cjs hook to override the yargs dependency version to 18.0.0 when running on Node.js version 24 or higher, and updates the CI test script to use this pnpmfile during installation. The review feedback suggests simplifying the Node.js version parsing using process.versions.node and removing verbose logging to prevent flooding the installation logs.
jskeet
approved these changes
Jun 9, 2026
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.
Recently, Node.js v24 and v26 were added to the GHA test matrix (#8445). However, the test runners (
c8andmocha) both depend on[email protected].In Node.js >= 24, when a package specifies
"type": "module"in itspackage.json, Node.js treats extensionless files inside that package as ES Modules (ESM). Since[email protected]contains an extensionless file namedyargsthat uses CommonJSrequire(), booting upc8ormochaon Node >= 24 crashes immediately with:We cannot simply upgrade
yargsto18.0.0globally becauseyargs@18is ESM-only, which would immediately break our test suite on Node 18 (our current minimum published version) because Node 18 does not supportrequire(esm)by default.The Solution
To support both Node 18 and Node 24/26 concurrently during this transitional period, we implement a dynamic
pnpmfilehook:.pnpmfile.cjs: Added a root-levelreadPackagehook that dynamically scans package dependencies. If it detectsyargsand the running Node.js version is Node >= 24, it dynamically overridesyargsto18.0.0(which is fully ESM-compatible and doesn't crash). On Node 18/20/22, it leaves it at17.7.2to ensure backward compatibility.ci/run_single_test.sh): Updated the test runner script to pass the--pnpmfileflag pointing to the root.pnpmfile.cjsduring all package installations.This is a zero-risk, highly-isolated, transitional solution. Once Node 18 is officially retired, this patch can be easily removed by running
pnpm patch-remove yargsand upgrading toyargs@18globally.Related Pull Requests