fix(cjs): recognize bracket/computed-string-literal export forms (#5275)#5276
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe CJS wrap pipeline in ChangesBracket-notation CJS detection and export extraction
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry/src/commands/compile/cjs_wrap/detect.rs`:
- Around line 83-91: The has_bracket_cjs_export function applies regex patterns
directly to raw source code, causing false positives when the patterns appear in
comments or strings. Modify the function to preprocess the source code to remove
comments (while preserving string literals for accurate quoted-key matching), or
add a lexical guard that validates regex matches are not within comment blocks
before returning true. This ensures that patterns like module['exports'] in
comments like // module['exports'] = x are not incorrectly classified as real
CommonJS exports.
- Around line 86-89: The regex patterns for detecting CommonJS exports don't
account for optional whitespace between the identifier and the opening bracket,
so they miss valid JavaScript like `module ['exports'] = …` and `exports
['name'] = …`. Update both regex patterns: in the first pattern that currently
requires `module[` with no gap, add `\s*` after `module` to allow optional
whitespace before the bracket, and in the second pattern that requires
`exports[` with no gap, add `\s*` after `exports` to allow optional whitespace
before the bracket.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 552e79ed-bf4d-40f4-b3a0-c132171550cb
📒 Files selected for processing (3)
crates/perry/src/commands/compile/cjs_wrap/detect.rscrates/perry/src/commands/compile/cjs_wrap/extract_exports.rscrates/perry/src/commands/compile/cjs_wrap/mod.rs
Comment on lines
+83
to
+91
| fn has_bracket_cjs_export(source: &str) -> bool { | ||
| // `module['exports'] = …` / `module["exports"] = …` (default export). | ||
| let module_default = | ||
| regex::Regex::new(r#"\bmodule\[\s*['"]exports['"]\s*\]\s*="#).unwrap(); | ||
| // `exports['name'] = …` / `module.exports['name'] = …` (named export). | ||
| let named = | ||
| regex::Regex::new(r#"\bexports\[\s*['"][A-Za-z_$][A-Za-z0-9_$]*['"]\s*\]\s*="#).unwrap(); | ||
| module_default.is_match(source) || named.is_match(source) | ||
| } |
There was a problem hiding this comment.
Bracket CJS detection can false-positive on comment/string text.
The new matcher runs on raw source, so text like // module['exports'] = x (or a string literal containing that snippet) can classify the file as CJS and force wrapping even when no real export assignment exists.
Please run the bracket matcher on a comment-stripped view (while preserving string-literal text needed for quoted-key matching), or add a lightweight lexical guard before accepting regex hits.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry/src/commands/compile/cjs_wrap/detect.rs` around lines 83 - 91,
The has_bracket_cjs_export function applies regex patterns directly to raw
source code, causing false positives when the patterns appear in comments or
strings. Modify the function to preprocess the source code to remove comments
(while preserving string literals for accurate quoted-key matching), or add a
lexical guard that validates regex matches are not within comment blocks before
returning true. This ensures that patterns like module['exports'] in comments
like // module['exports'] = x are not incorrectly classified as real CommonJS
exports.
added 2 commits
June 17, 2026 05:09
`module['exports'] = X` / `module["exports"] = X` and `exports['name'] = X` / `exports["name"] = X` are semantically identical to the dot forms but were not recognized as CJS export assignments, so a file using them (e.g. @colors/colors's lib/custom/trap.js: `module['exports'] = function runTheTrap`) fell through to the ESM pipeline — bare `module`/`exports` threw at init and the default export never registered. This blocked winston (via @colors/colors). Two-part fix, both in the cjs_wrap layer: 1. Detection (detect.rs): is_commonjs now recognizes the bracket forms. strip_comments_and_strings blanks the quoted key, so the match runs on the original source via a regex that requires an `=` (assignment) and a string-literal key — a dynamic `module[k] = X` stays ESM. 2. Extraction (extract_exports.rs): extract_single_module_exports_assignment accepts `module['exports'] = Ident` (class-identity default path), and extract_exports_from_source surfaces `exports['name']`/`module.exports['name']` named exports. Dynamic (non-literal) keys are not extracted. Inside the IIFE wrap, `module`/`exports` are ordinary locals, so the bracket assignment executes correctly at runtime and `export default _cjs` / `export const name = _cjs.name` resolve — no HIR lowering change needed. Tests: 8 new cjs_wrap unit tests; e2e mb repro (`hi x`), named `exports['foo']` (`foo x`), double-quote `module["exports"]` (`dq x`), dot-form regression, dynamic-key regression — all match Node. No version/CHANGELOG/Cargo.lock edits (maintainer folds in at merge).
proggeramlug
force-pushed
the
fix/cjs-bracket-exports
branch
from
b900da9 to
148f896
Compare
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
1 participant
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.
Root cause (#5275)
Perry recognized the dot forms
module.exports = …andexports.foo = …as CommonJS export assignments, but not the bracket / computed-string-literal formsmodule['exports'] = …/module["exports"] = …(default) andexports['name'] = …/exports["name"] = …(named).@colors/colors/lib/custom/trap.jsdoesmodule['exports'] = function runTheTrap(…){…}. Because that file was not recognized as CJS, it fell through to the ESM-only pipeline: the baremodule/exportsidentifiers threw at module init (module is not defined) and the default export was never registered. This blocked winston (which depends on@colors/colors).Fix
Two-part, both confined to the
cjs_wraplayer (crates/perry/src/commands/compile/cjs_wrap/):detect.rs):is_commonjsnow recognizes the bracket export forms. Becausestrip_comments_and_stringsblanks the quoted key (module['exports']→module[ ]), the newhas_bracket_cjs_exporthelper matches on the original source via a regex that requires an=(a real assignment) and a string-literal key. A genuinely dynamicmodule[k] = …(non-literal key) does not match and stays on the ESM/runtime path.extract_exports.rs):extract_single_module_exports_assignmentacceptsmodule['exports'] = Ident(the class-identity default path), equivalent tomodule.exports = Ident.extract_exports_from_sourcesurfacesexports['name'] = …/module.exports['name'] = …as named exports (with the same.-boundary guard the dot matcher uses to avoid inner-modulee.exports['X']). Dynamic keys are not extracted.No HIR lowering change is needed: inside the IIFE wrap,
module/exportsare ordinary localvars, so the bracket assignment executes correctly at runtime andexport default _cjs;/export const name = _cjs.name;resolve.Test evidence
cjs_wrapunit tests (detection, extraction, dynamic-key rejection, end-to-end wrap+parse). Fullcjs_wrapsuite: 83 passed, 0 failed.node main.ts):module['exports'] = function greetdefault require →hi xexports['foo'] = …named import →foo xmodule["exports"]→dq xmodule.exports+module.exports.extra→dot x extra yo[k]=…object access in a CJS module →42winston before/after
@colors/colorsbracket export not recognized →module is not defined.PERRY_NO_AUTO_OPTIMIZE=1 perry drivers/winston_.ts -o /tmp/w→Wrote executable. Running it gets past the@colors/colorswall; the next (unrelated, out-of-scope) wall isSyntaxError: Invalid regular expression: /[0m/: invalid pattern(an ANSI-escape regex-engine gap). Not fixed here.Notes
[workspace.package] version,Current Version,CHANGELOG.md, orCargo.lockedits — the maintainer folds version + changelog in at merge.cjs_wrap/detect.rs,cjs_wrap/extract_exports.rs,cjs_wrap/mod.rs.Risk
Low. Detection only widens to additional CJS signals (gated by an
=assignment and a string-literal key;has_top_level_esmstill wins first, so real ESM files are unaffected). Extraction reuses the existing dot-matcher boundary guards. Dynamic non-literal keys are explicitly excluded and verified.Summary by CodeRabbit
Bug Fixes
module['exports']/module["exports"]andexports['name']/exports["name"].module[k]) are still not treated as CommonJS signals.Tests