fix(commonjs): recognize named export#1962
Conversation
|
@rtritto looks like CI is failing on prettier |
|
I reviewed the diff + CI + the context in #1678. Using MUST fix (before merge)
IMPROVEMENTS (non-blocking) |
|
@shellscape thanks for reply |
rtritto
force-pushed
the
analyze-exports-lexer
branch
from
574b39e to
d4f6c3c
Compare
|
@shellscape please can you review? |
There was a problem hiding this comment.
Pull request overview
This PR enhances @rollup/plugin-commonjs named-export detection by integrating cjs-module-lexer, aiming to recognize additional CommonJS export patterns (e.g. Object.defineProperty(exports, "foo", ...)) and expose them as ESM named exports.
Changes:
- Add
cjs-module-lexerand a new lexer-based export analysis helper. - Populate
commonjsmodule meta with lexer-detected exports and use them to generate additional named exports in proxies and transforms. - Add a new fixture and test intended to cover named CJS exports.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Locks the newly added cjs-module-lexer dependency. |
| packages/commonjs/package.json | Adds cjs-module-lexer to runtime dependencies. |
| packages/commonjs/src/analyze-exports-lexer.js | Introduces lexer initialization and export analysis utilities. |
| packages/commonjs/src/index.js | Runs lexer analysis during transform, stores results in module meta, and initializes lexer in buildStart. |
| packages/commonjs/src/transform-commonjs.js | Augments the generated export block with lexer-detected named exports not found via AST. |
| packages/commonjs/src/proxies.js | Extends ES import proxies to explicitly re-export lexer-detected named exports. |
| packages/commonjs/test/test.js | Adds a new test for named exports from CJS (currently written in an incompatible style for Vitest). |
| packages/commonjs/test/fixtures/samples/named-cjs-exports/main.cjs | Adds a new sample module with named exports. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| test('test named exports from cjs', async (t) => { | ||
| const bundle = await rollup({ | ||
| input: 'fixtures/samples/named-cjs-exports/main.cjs', | ||
| plugins: [commonjs()] | ||
| }); |
There was a problem hiding this comment.
The PR description mentions supporting named exports created via Object.defineProperty(exports, "foo", …), but the added fixture/test only covers assignment-based exports. Add a fixture that defines a named export via Object.defineProperty and assert it is recognized to prevent regressions for the reported issue.
| buildStart({ plugins }) { | ||
| async buildStart({ plugins }) { | ||
| validateVersion(this.meta.rollupVersion, peerDependencies.rollup, 'rollup'); | ||
| await ensureLexerInit(); |
There was a problem hiding this comment.
buildStart now unconditionally awaits ensureLexerInit(). If cjs-module-lexer initialization fails in an environment (WASM load issues, restricted FS, etc.), the entire plugin will hard-fail before any fallback logic can run. Consider catching initialization errors here (and warning) so the plugin can continue with the existing AST-based export detection.
| await ensureLexerInit(); | |
| try { | |
| await ensureLexerInit(); | |
| } catch (error) { | |
| this.warn({ | |
| code: 'CJS_LEXER_INIT_FAILED', | |
| message: | |
| 'Failed to initialize cjs-module-lexer. Falling back to AST-based export detection.' + | |
| (error && error.message ? ` Cause: ${error.message}` : '') | |
| }); | |
| } |
| await ensureInit(); | ||
| try { |
There was a problem hiding this comment.
analyzeExports calls await ensureInit() outside the try block, so any failure to initialize the lexer (e.g. WASM load/init errors) will reject and bypass the intended graceful fallback. Wrap initialization in the same try/catch (or have ensureInit return a cached init promise and handle errors) so lexer failures don’t hard-fail the build.
| await ensureInit(); | |
| try { | |
| try { | |
| await ensureInit(); |
| // If lexer fails (e.g. WASM issue), fall back gracefully | ||
| this.warn( | ||
| `[commonjs] cjs-module-lexer failed for ${id}: ${err.message}. ` + | ||
| 'Falling back to no named exports.' | ||
| ); | ||
|
|
||
| return { exports: [], reexports: [], hasDefaultExport: true }; |
There was a problem hiding this comment.
analyzeExports uses this.warn(...), but this function is not called with a Rollup plugin context (it’s invoked as analyzeExports(code, id)), so this will be undefined and this path will throw. Pass a warn callback/context into analyzeExports (or move the warning to the caller) and ensure the fallback return does not incorrectly set hasDefaultExport: true when lexing fails.
|
|
||
| /** | ||
| * Ensure cjs-module-lexer WASM is initialized. | ||
| * Safe to call multiple times — will only init once. | ||
| */ | ||
| export async function ensureInit() { | ||
| if (!initialized) { | ||
| await init(); | ||
| initialized = true; | ||
| } |
There was a problem hiding this comment.
ensureInit is not concurrency-safe: if two async callers hit it before initialized flips to true, init() can run multiple times. Consider caching the in-flight init promise (e.g. let initPromise) or setting the flag before awaiting to ensure only one initialization occurs per process.
| /** | |
| * Ensure cjs-module-lexer WASM is initialized. | |
| * Safe to call multiple times — will only init once. | |
| */ | |
| export async function ensureInit() { | |
| if (!initialized) { | |
| await init(); | |
| initialized = true; | |
| } | |
| let initPromise = null; | |
| /** | |
| * Ensure cjs-module-lexer WASM is initialized. | |
| * Safe to call multiple times — will only init once. | |
| */ | |
| export async function ensureInit() { | |
| if (initialized) return; | |
| if (!initPromise) { | |
| initPromise = (async () => { | |
| await init(); | |
| initialized = true; | |
| initPromise = null; | |
| })(); | |
| } | |
| await initPromise; |
| test('test named exports from cjs', async (t) => { | ||
| const bundle = await rollup({ | ||
| input: 'fixtures/samples/named-cjs-exports/main.cjs', | ||
| plugins: [commonjs()] | ||
| }); | ||
|
|
||
| t.plan(3); | ||
| await testBundle(t, bundle); |
There was a problem hiding this comment.
This test is executed by Vitest (see package.json), which does not pass an AVA-style t object into the test callback. As written, t will be undefined and t.plan(3) will throw, failing the suite. Align this test with the surrounding ones (e.g. expect.assertions(…) + testBundle(avaAssertions, bundle)), or use Vitest’s test context APIs.
| test('test named exports from cjs', async (t) => { | |
| const bundle = await rollup({ | |
| input: 'fixtures/samples/named-cjs-exports/main.cjs', | |
| plugins: [commonjs()] | |
| }); | |
| t.plan(3); | |
| await testBundle(t, bundle); | |
| test('test named exports from cjs', async () => { | |
| const bundle = await rollup({ | |
| input: 'fixtures/samples/named-cjs-exports/main.cjs', | |
| plugins: [commonjs()] | |
| }); | |
| expect.assertions(3); | |
| await testBundle(avaAssertions, bundle); |
|
@sapphi-red @bluwy @tada5hi @TrickyPi sorry for the ping, please can you take a look? |
|
@CharlieHelps please do a thorough review of this PR to determine if it's ready to merge. Look at the diff, reviews, comments, and referenced issues/docs/links to build a complete understanding. Then respond with a comment that includes a list of MUST fix items that must be fixed before the the PR is merged, and a list of IMPROVEMENTS that shouldn't block the merge but may be quick improvements or things that could be addressed in a follow up PR. The MUST fix items should explain exactly what's wrong and what needs to change to resolve the problem. Both should be numbered lists and the IMPROVEMENTS list should continue counting up from the MUST list. It's OK if there are no IMPROVEMENTS or MUST fix items, you can just omit the lists. @rtritto looks like you've got to run linting and formatting on this PR |
|
I did a full pass over the diff, existing reviews/comments, linked issue #1678, and current CI. This PR is not ready to merge yet. MUST fix (blocking)
IMPROVEMENTS (non-blocking, follow-up-friendly)
|
Rollup Plugin Name:
@rollup/plugin-commonjsThis PR contains:
Are tests included?
Breaking Changes?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers: #1678
Description
Add the analyze export lexer for named export that allows to recognize exports like: