fix(transformer): use require not import in CommonJS files#18226
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in the transformer where CommonJS files were incorrectly receiving import statements instead of require() calls. The issue was that the code was checking source_type.is_script() which returns false for CommonJS files (they have ModuleKind::CommonJS, not ModuleKind::Script). The fix changes the check to source_type.is_module(), which correctly identifies ES Modules vs everything else (Script, CommonJS).
Changes:
- Swapped the logic in JSX transformer and module imports to use
is_module()instead ofis_script() - Removed an unused helper method
is_script()fromJsxImpl - Fixed test snapshot showing one additional passing test
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
crates/oxc_transformer/src/jsx/jsx_impl.rs |
Swapped conditional logic from is_script() to is_module() for determining whether to use AutomaticModule vs AutomaticScript bindings, and removed the unused is_script() helper method |
crates/oxc_transformer/src/common/module_imports.rs |
Fixed the conditional logic to use is_module() instead of is_script() when deciding whether to insert import statements or require statements |
tasks/coverage/snapshots/semantic_babel.snap |
Updated test snapshot showing one additional passing test and removed the error for a CommonJS file that was incorrectly using import symbol flags |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I think this should fix the problem. Am running https://github.com/oxc-project/monitor-oxc/actions/runs/21135903246 |
|
Yes, the fix works. |
Merge activity
|
Transformer was deciding whether to insert an `import` statement or `require()` call depending on `source_type.is_script()`. That returns `false` for CommonJS, which was resulting in `import` statements being inserted in CommonJS files. Fix it by checking `source_type.is_module()` instead.
c4fa5d5 to
90e5f08
Compare
Transformer was deciding whether to insert an `import` statement or `require()` call depending on `source_type.is_script()`. That returns `false` for CommonJS, which was resulting in `import` statements being inserted in CommonJS files. Fix it by checking `source_type.is_module()` instead.
90e5f08 to
645c3f0
Compare
### 🐛 Bug Fixes - f1e2dc0 semantic: No error in `check_function_redeclaration` for CommonJS files (#18231) (overlookmotel) - 645c3f0 transformer: Use `require` not `import` in CommonJS files (#18226) (overlookmotel) - ee9f6a4 mangler: Use `retain` instead of `truncate` to remove empty frequency slots (#18225) (Dunqing) ### ⚡ Performance - 52073d9 semantic: Use cheaper test for source type (#18235) (overlookmotel) Co-authored-by: overlookmotel <557937+overlookmotel@users.noreply.github.com>

Transformer was deciding whether to insert an
importstatement orrequire()call depending onsource_type.is_script(). That returnsfalsefor CommonJS, which was resulting inimportstatements being inserted in CommonJS files. Fix it by checkingsource_type.is_module()instead.