Repository navigation
Conversation
The parser lifts `exports.a = 1` to `var $a = 1` and counts no use of `exports` for it. When the file is wrapped after all, the printer writes `exports.a = 1` again, so the wrapper needs the `exports` parameter. Link step 4 gave that parameter only to files with the `COMMONJS_LIFTED_TO_ESM` flag. A file that the parser itself made CommonJS has no such flag. If the code that made it CommonJS is dead or its statement is removed, no use of `exports` is counted and the wrapper had no parameter: `ReferenceError: exports is not defined`. - Step 4 now also gives `exports` to a wrapped file that has lifted exports and neither the `exports` nor the `module` parameter. - The parser keeps the wrapper when `exports` was assigned and `module` is still used. The printer then writes `module.exports` as written, and a lifted file has no `module`. - A debug assertion in `print_code_for_file_in_chunk_js` holds the rule.
…wrapper-exports-parameter
|
Status: ready for review. CI passed on 3abb9c0 (build 134815). How I reproduced it, on main: printf 'exports.a = 1;\nif (process.browser) {\n this.b = 1;\n}\n' > lib.cjs
printf 'import lib from "./lib.cjs";\nconsole.log(lib.a);\n' > entry.js
bun build entry.js --outdir=out --target=bun && bun out/entry.jsThe bundle throws The 12 new cases in CI notes: 193 jobs passed. The two PR: #46565 |
|
Updated 12:18 AM PT - Oct 29th, 2026
❌ @robobun, your commit 3abb9c0 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 46565That installs a local version of the PR into your bun-46565 --bun |
…tests The parser rule kept the wrapper only for a file with a dead or removed `exports = ...`. The same `module.exports` read without that assignment still prints a name that nothing declares, and a file with a `module.exports.x = ...` store lost its lift. Remove the rule and its tests. The linker rule stays. `extra/DefineExports1`, `extra/DefineExports2` and `extra/DefineExportsArray2` failed with `exports is not defined` and pass with the linker rule, so they are no longer `todo`.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
The name replaces a two-line comment. Shorten two test comments.
robobun
changed the title
exports and module that its lifted statements printexports in a CommonJS wrapper that prints lifted exports
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 7 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked that the widened step-4 condition cannot steal inputs from the lifted-setter else if (the new disjunct requires COMMONJS_LIFTED_TO_ESM to be unset, and the setter branch requires it set), and that every wrap = WrapKind::Cjs assignment lives in steps 1-2 of scanImportsAndExports.rs, so no row reaches the new debug_assert! in LinkerContext.rs without passing the step-4 loop.
Extended reasoning...
The change widens one linker condition in src/bundler/linker_context/scanImportsAndExports.rs so a Cjs-wrapped file with lifted named exports and no counted exports/module use gets the exports wrapper parameter, adds a debug-only invariant in src/bundler/LinkerContext.rs, un-todos three esbuild-ported tests, and adds a matrix of itBundled cases. It touches no security-sensitive surface. The one posted finding is a pre-existing sibling gap in doStep5 that the author already lists as out of scope, so a human should weigh whether that is acceptable to land separately.
|
Merged main (37a5e3a) into this branch. The PR merges clean again.
|
Problem
bun build --target=bunturnsexports.a = 1; if (process.browser) { this.b = 1; }into__commonJS(function() { exports.a = 1; ... }):ReferenceError: exports is not defined.exports.a = 1and counts no use ofexports. A top-levelthisin dead or removed code still marks the file CommonJS (src/js_parser/p.rs:5949). Link step 4 addsexportsonly forCOMMONJS_LIFTED_TO_ESMfiles (scanImportsAndExports.rs:626).Fix
exportsto every wrapped file with lifted exports and no parameter: the rule of bundler: no export table for a lifted CommonJS file that stays wrapped #41334 on the printer's condition,wrap == Cjs.test/bundler/bundler_cjs2esm.test.ts, 12 new cases fail on main. Threetodotests inesbuild/extra.test.tsnow pass.module.exports(Notes).Background
exports.a = 1intovar $a = 1. A wrapped file printsexports.a = 1again.Downsides
.text: +256 bytes. 0 of 32,425 corpus outputs change.module.exportsread in a lifted file (bundler: keep the CommonJS wrapper when the lift leaves amodule.exportsvalue behind #35665), and three more shapes (Notes).Notes
Repro
On main the bundle throws
ReferenceError: exports is not defined. Node prints 1 for the source.process.browserisfalsefor--target=bunand--target=node, so the branch is dead. The same happens forif (process.env.NODE_ENV !== "production") { console.log(this === exports); }with--define 'process.env.NODE_ENV="production"'. With--productionthe message isReferenceError: O is not defined. A--target=nodebundle throws undernode(exports is not defined in ES module scope). Underbunit does not throw and printsundefinedfor the import.Where it comes from. No GitHub issue and no package that I know of. I found it by reading the code around #45345. Three tests ported from esbuild have recorded it as
todosince they were added:extra/DefineExports1,extra/DefineExports2andextra/DefineExportsArray2(in.jsends withthis;).Inputs. Each line follows
exports.a = 1;. The import form does not matter: default, named and star import,require(), a splitimport(), and the file as an entry point all fail.if (false) { console.log(this); }exports.b = true ? 2 : this;exports.b = 0 || 2 || this;false && this.x;function f() { return 1; } if (false) { f.call(this); }this;(() => this);(function () { module.exports(); });(function () { module.exports = {}; });(function () { delete exports.a; });(function () { delete module.exports; });(function () { delete module.exports.a; });I ran these on the 1.0.0, 1.1.0, 1.2.0, 1.3.0, 1.4.0 and 1.4.1 release binaries and on main. Up to 1.2.0 the message is
ReferenceError: Can't find variable: exports. The filenode_modules/react/index.jswithimport { z } from "./z.js"; exports.a = z;, default-imported, also fails since 1.4.1. Node rejects that source, so the reference for it is Bun up to 1.4.0.Mechanism
value_for_thisrecords a use ofexportsand setscommonjs_named_exports_deoptimized.record_usagedoes nothing in dead control flow. For a removed statement,append_parttakes the use back. The flag stays in both cases._parseadds uses ofexportsfor a deoptimized file only on the path that deoptimizes at the end of the visit (parse_entry.rs:1592). That path is skipped when the flag is already set.uses_exports_refis then false while the file isExportsKind::Cjs.generateCodeForFileInChunkJS.rs:545) adds parameters only forUSES_EXPORTS_REForUSES_MODULE_REF. The printer prints a lifted export asexports.namewheneverwrap == Cjs.require()of a lifted file). Those haveCOMMONJS_LIFTED_TO_ESM. A file that the parser made CommonJS does not. Thereactfile above does not either: itsimportkeeps the flag off, and the linker wraps it for the default import.wrap == Cjs, neither flag set, and a table of lifted exports that is not empty. A file with either flag already hasexports, and its output does not change (the minifier counts one more use ofexportsfor the flag).debug_assert!inprint_code_for_file_in_chunk_jssits where the printer option is set: a wrapped file with lifted exports hasUSES_EXPORTS_REForUSES_MODULE_REF.Designs that were weighed
append_partwhen a statement is removed, and guardvalue_for_thisagainst dead code. It makes these files lifted and does not reach a file that only the linker wraps. A lifted file is not always right today:exports.a = 1; this; exports.b = Object.keys(module.exports).join();runs as an entry point now, and without the mark ofthis;it prints an undeclaredexports_lib(bundler: keep the CommonJS wrapper when the lift leaves amodule.exportsvalue behind #35665).exportsin_parsefor every deoptimized file. The parser cannot seewrap, so the linker-wrappedreactfile still throws.module, in every output format.Not fixed here. Each input follows
exports.a = 1;and throws on main and on this branch.exports.b = Object.keys(module.exports).join();with a named import:exports_lib is not defined. Amodule.exportsread in a lifted file prints a name that nothing declares. bundler: keep the CommonJS wrapper when the lift leaves amodule.exportsvalue behind #35665 is open for it (Compile Issue w/ @apollo/client #12518, bun build generates undefined variable when bundling @swc/wasm-typescript #14939).if (false) { exports = {}; } exports.b = typeof module.exports;:module_lib is not defined. The dead assignment sets a second mark and the same read printsmodule_lib.exports. The first version of this PR kept the wrapper for such a file with one more test in_parse. The self-review found that rule too narrow (the input above still threw) and too wide (a file with amodule.exports.x = ...store lost its lift), so it is out. It belongs with bundler: keep the CommonJS wrapper when the lift leaves amodule.exportsvalue behind #35665.(function () { return module.exports.a; }); exports.b = typeof module.constructor;:module_lib is not defined. js_parser: roll back a removed statement's tracked uses along with its use counts #45345 rolls back the count of a removedmodule.exportsread.exportstatement and lifted exports:$a is not defined. Step 5 (doStep5.rs:149) still builds__export(exports, { a: () => $a })getters for the lifted names unless the file hasCOMMONJS_LIFTED_TO_ESM. Input:lib.jswithimport { z } from "./z.js"; exports.a = z; export * from "./dyn.cjs";, default-imported. No PR is open for it.exportsparameter that it does not use.Related open PRs. #42500 applies the unwrap list only to CommonJS files, which changes how the
reactfile of the last test is classified. #45345, #42595 and #41635 change the parser's decision to wrap. None of them changes the wrapper parameters.Measurements. Release builds of main 5c6d4e2 with and without this diff.
valgrindandperfare not installed, so instruction counts are gdb single-step counts..textscan_imports_and_exportsParser::_parse, JS and TypeScriptprint_code_for_file_in_chunk_jsexportsexportsexportsexports.js,.cjsand.mjsfile of 1,165 package names undertest/node_modulesandnode_modules. Each is built withbun build <file> --target=node --packages=external, and the sha256 of each output is compared. 63 files fail to build in both binaries. The first version of this diff had the same linker change. For it I also ran--minify-identifiers,--minifyand--format=cjs: 0 outputs differ in each. This does not cover first-party CommonJS under an application's own defines.Tests
test/bundler/bundler_cjs2esm.test.ts: 12 new cases. The 12 inputs of the table run through a default, named and star import andrequire(). One more case adds six files that must keep their shape. The others: an entry point,--minify-identifiers,--splitting, theNODE_ENVandprocess.browsertriggers, and the linker-wrappedreactfile.test/bundler/esbuild/extra.test.ts:todo: trueremoved from the three tests named above. No other existing expectation is edited.test/bundler/: the 12 new cases and the threeextratests fail without it and pass with it.bytecode cache portability > encoder output is identical on every platformtimes out with both on my machine. No other result differs.bundler_cjs2esm125 pass.esbuild/extra.test.ts225 pass, 9 todo.bundler_cjs,bundler_edgecase,bundler_regressions,react-compiler,bundler_npm,bundler_splitting,bundler_browser,bundler_minify: 681 pass. The new assertion did not fire.