Conversation
|
Looks like it only works with node.js >= 22. And we need some workarounds for pkg. So, maybe we'll add support for this, when we drops support for node.js 18 and 20. |
FWIW if the issue is loading ESM modules with |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for loading ESM pnpmfiles with the .mjs extension alongside existing CommonJS .cjs support. This enhancement allows pnpm users to write their configuration hooks using ES module syntax.
- Converting synchronous pnpmfile loading to asynchronous to support dynamic ES module imports
- Adding ESM detection and loading logic based on
.mjsfile extension - Updating all calling code to handle the new async interface
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| hooks/pnpmfile/src/requirePnpmfile.ts | Core logic to detect and load ESM vs CommonJS pnpmfiles |
| hooks/pnpmfile/src/requireHooks.ts | Made function async and updated to handle Promise.all for parallel loading |
| hooks/pnpmfile/test/index.ts | Updated all tests to use async/await syntax |
| pkg-manager/plugin-commands-installation/src/recursive.ts | Added await for async requireHooks call |
| cli/cli-utils/src/getConfig.ts | Added await for async requireHooks call |
| pnpm/test/hooks.ts | Added integration test for ESM pnpmfile loading |
| .changeset/real-papers-cut.md | Changelog entry documenting the new feature |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (pnpmFilePath.endsWith('.mjs')) { | ||
| const url = pathToFileURL(path.resolve(pnpmFilePath)).href | ||
| const module = await import(url) | ||
| pnpmfile = module.default || module |
There was a problem hiding this comment.
The fallback logic module.default || module may be unclear. Consider being more explicit about when to use default (for default exports) vs the module itself (for named exports like export const hooks = {}). The current logic could mask issues if both exist.
| pnpmfile = module.default || module | |
| if ('default' in module && Object.keys(module).length > 1) { | |
| throw new PnpmFileFailError( | |
| pnpmFilePath, | |
| new Error( | |
| 'The pnpmfile exports both a default export and named exports. This is ambiguous. Please export either a default or named exports, not both.' | |
| ) | |
| ) | |
| } | |
| pnpmfile = 'default' in module ? module.default : module |
| const pnpmFileRealName = pnpmFilePath.endsWith('.cjs') || pnpmFilePath.endsWith('.mjs') | ||
| ? pnpmFilePath | ||
| : `${pnpmFilePath}.cjs` | ||
| return fs.existsSync(pnpmFileRealName) |
There was a problem hiding this comment.
The function pnpmFileExistsSync only falls back to .cjs extension when neither .cjs nor .mjs extensions are present. This means .mjs files without explicit extension won't be found, which could be inconsistent behavior. Consider adding fallback logic for .mjs files as well.
| const pnpmFileRealName = pnpmFilePath.endsWith('.cjs') || pnpmFilePath.endsWith('.mjs') | |
| ? pnpmFilePath | |
| : `${pnpmFilePath}.cjs` | |
| return fs.existsSync(pnpmFileRealName) | |
| if (pnpmFilePath.endsWith('.cjs') || pnpmFilePath.endsWith('.mjs')) { | |
| return fs.existsSync(pnpmFilePath) | |
| } | |
| return fs.existsSync(`${pnpmFilePath}.cjs`) || fs.existsSync(`${pnpmFilePath}.mjs`) |
Related issue: yao-pkg/pkg#144