mirror of
https://github.com/oven-sh/bun
synced 2026-02-02 15:08:46 +00:00
fix(bundler): legal comments no longer break module.exports = require() redirect optimization (#26113)
## Summary
- Legal comments (`/*! ... */`) were preventing the `module.exports =
require()` redirect optimization from being applied to CommonJS wrapper
modules
- The fix scans all parts to find a single meaningful statement,
skipping comments, directives, and empty statements
- If exactly one such statement exists and matches the `module.exports =
require()` pattern, the redirect optimization is now applied
This fixes an issue where wrapper modules like Express's `index.js`:
```js
/*!
* express
* MIT Licensed
*/
'use strict';
module.exports = require('./lib/express');
```
Were generating unnecessary wrapper functions instead of being
redirected directly to the target module.
## Test plan
- [x] Added regression test in `test/regression/issue/3179.test.ts`
- [x] Verified test fails with system bun and passes with the fix
- [x] Tested manual reproduction scenario
Fixes #3179
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Bot <claude-bot@bun.sh>
Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -783,9 +783,39 @@ pub const Parser = struct {
|
||||
// else
|
||||
// module.exports = require('./foo.dev.js')
|
||||
//
|
||||
if (parts.items.len == 1 and parts.items[0].stmts.len == 1) {
|
||||
var part = &parts.items[0];
|
||||
const stmt: Stmt = part.stmts[0];
|
||||
// Find the part containing the actual module.exports = require() statement,
|
||||
// skipping over parts that only contain comments, directives, and empty statements.
|
||||
// This handles files like:
|
||||
//
|
||||
// /*!
|
||||
// * express
|
||||
// * MIT Licensed
|
||||
// */
|
||||
// 'use strict';
|
||||
// module.exports = require('./lib/express');
|
||||
//
|
||||
// When tree-shaking is enabled, each statement becomes its own part, so we need
|
||||
// to look across all parts to find the single meaningful statement.
|
||||
const StmtAndPart = struct { stmt: Stmt, part_idx: usize };
|
||||
const stmt_and_part: ?StmtAndPart = brk: {
|
||||
var found: ?StmtAndPart = null;
|
||||
for (parts.items, 0..) |part, part_idx| {
|
||||
for (part.stmts) |s| {
|
||||
switch (s.data) {
|
||||
.s_comment, .s_directive, .s_empty => continue,
|
||||
else => {
|
||||
// If we already found a non-trivial statement, there's more than one
|
||||
if (found != null) break :brk null;
|
||||
found = .{ .stmt = s, .part_idx = part_idx };
|
||||
},
|
||||
}
|
||||
}
|
||||
}
|
||||
break :brk found;
|
||||
};
|
||||
if (stmt_and_part) |found| {
|
||||
const stmt = found.stmt;
|
||||
var part = &parts.items[found.part_idx];
|
||||
if (p.symbols.items[p.module_ref.innerIndex()].use_count_estimate == 1) {
|
||||
if (stmt.data == .s_expr) {
|
||||
const value: Expr = stmt.data.s_expr.value;
|
||||
@@ -800,13 +830,13 @@ pub const Parser = struct {
|
||||
left.data.e_dot.target.data == .e_identifier and
|
||||
left.data.e_dot.target.data.e_identifier.ref.eql(p.module_ref))
|
||||
{
|
||||
const redirect_import_record_index: ?u32 = brk: {
|
||||
const redirect_import_record_index: ?u32 = inner_brk: {
|
||||
// general case:
|
||||
//
|
||||
// module.exports = require("foo");
|
||||
//
|
||||
if (right.data == .e_require_string) {
|
||||
break :brk right.data.e_require_string.import_record_index;
|
||||
break :inner_brk right.data.e_require_string.import_record_index;
|
||||
}
|
||||
|
||||
// special case: a module for us to unwrap
|
||||
@@ -825,10 +855,10 @@ pub const Parser = struct {
|
||||
{
|
||||
// We know it's 0 because there is only one import in the whole file
|
||||
// so that one import must be the one we're looking for
|
||||
break :brk 0;
|
||||
break :inner_brk 0;
|
||||
}
|
||||
|
||||
break :brk null;
|
||||
break :inner_brk null;
|
||||
};
|
||||
if (redirect_import_record_index) |id| {
|
||||
part.symbol_uses = .{};
|
||||
|
||||
159
test/regression/issue/3179.test.ts
Normal file
159
test/regression/issue/3179.test.ts
Normal file
@@ -0,0 +1,159 @@
|
||||
import { expect, test } from "bun:test";
|
||||
import { bunEnv, bunExe, tempDir } from "harness";
|
||||
|
||||
// https://github.com/oven-sh/bun/issues/3179
|
||||
// Legal comments (/*!...*/) should not prevent the module.exports = require() redirect optimization
|
||||
|
||||
test("legal comments do not break module.exports = require() redirect", async () => {
|
||||
using dir = tempDir("issue-3179", {
|
||||
// Wrapper module WITH legal comment
|
||||
"with-comment.js": `/*!
|
||||
* express
|
||||
* MIT Licensed
|
||||
*/
|
||||
|
||||
'use strict';
|
||||
|
||||
module.exports = require('./lib/express');
|
||||
`,
|
||||
// Wrapper module WITHOUT legal comment
|
||||
"without-comment.js": `'use strict';
|
||||
|
||||
module.exports = require('./lib/express');
|
||||
`,
|
||||
// The actual module being wrapped
|
||||
"lib/express.js": `function createApp() {
|
||||
return { name: 'app' };
|
||||
}
|
||||
module.exports = createApp;
|
||||
`,
|
||||
// Entry point using the module with legal comment
|
||||
"entry-with.js": `const express = require('./with-comment');
|
||||
console.log(express());
|
||||
`,
|
||||
// Entry point using the module without legal comment
|
||||
"entry-without.js": `const express = require('./without-comment');
|
||||
console.log(express());
|
||||
`,
|
||||
});
|
||||
|
||||
// Bundle both variants
|
||||
await using procWith = Bun.spawn({
|
||||
cmd: [bunExe(), "build", "entry-with.js", "--outfile=out-with.js"],
|
||||
cwd: String(dir),
|
||||
env: bunEnv,
|
||||
stdout: "pipe",
|
||||
stderr: "pipe",
|
||||
});
|
||||
|
||||
await using procWithout = Bun.spawn({
|
||||
cmd: [bunExe(), "build", "entry-without.js", "--outfile=out-without.js"],
|
||||
cwd: String(dir),
|
||||
env: bunEnv,
|
||||
stdout: "pipe",
|
||||
stderr: "pipe",
|
||||
});
|
||||
|
||||
const [stdoutWith, stderrWith, exitCodeWith] = await Promise.all([
|
||||
procWith.stdout.text(),
|
||||
procWith.stderr.text(),
|
||||
procWith.exited,
|
||||
]);
|
||||
|
||||
const [stdoutWithout, stderrWithout, exitCodeWithout] = await Promise.all([
|
||||
procWithout.stdout.text(),
|
||||
procWithout.stderr.text(),
|
||||
procWithout.exited,
|
||||
]);
|
||||
|
||||
expect(exitCodeWith).toBe(0);
|
||||
expect(exitCodeWithout).toBe(0);
|
||||
|
||||
// Read the generated bundles
|
||||
const outWith = await Bun.file(`${dir}/out-with.js`).text();
|
||||
const outWithout = await Bun.file(`${dir}/out-without.js`).text();
|
||||
|
||||
// Both bundles should have the same number of "// " module comment markers
|
||||
// If the redirect optimization is working, neither should create a wrapper function
|
||||
const moduleCountWith = (outWith.match(/\/\/ /g) || []).length;
|
||||
const moduleCountWithout = (outWithout.match(/\/\/ /g) || []).length;
|
||||
|
||||
// The key test: both should have the same number of modules bundled
|
||||
// Before the fix, "with-comment" would bundle 3 modules instead of 2
|
||||
expect(moduleCountWith).toBe(moduleCountWithout);
|
||||
|
||||
// Both should NOT contain a require_with_comment or require_without_comment wrapper
|
||||
expect(outWith).not.toContain("require_with_comment");
|
||||
expect(outWithout).not.toContain("require_without_comment");
|
||||
});
|
||||
|
||||
test("legal comment with only module.exports = require()", async () => {
|
||||
using dir = tempDir("issue-3179-simple", {
|
||||
"wrapper.js": `/*!
|
||||
* Legal comment
|
||||
*/
|
||||
module.exports = require('./target');
|
||||
`,
|
||||
"target.js": `module.exports = { foo: 'bar' };
|
||||
`,
|
||||
"entry.js": `const wrapper = require('./wrapper');
|
||||
console.log(wrapper.foo);
|
||||
`,
|
||||
});
|
||||
|
||||
await using proc = Bun.spawn({
|
||||
cmd: [bunExe(), "build", "entry.js", "--outfile=out.js"],
|
||||
cwd: String(dir),
|
||||
env: bunEnv,
|
||||
stdout: "pipe",
|
||||
stderr: "pipe",
|
||||
});
|
||||
|
||||
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
|
||||
|
||||
expect(exitCode).toBe(0);
|
||||
|
||||
const out = await Bun.file(`${dir}/out.js`).text();
|
||||
|
||||
// Should not contain a wrapper function - the redirect optimization should work
|
||||
expect(out).not.toContain("require_wrapper");
|
||||
});
|
||||
|
||||
test("multiple legal comments and directives do not break redirect", async () => {
|
||||
using dir = tempDir("issue-3179-multiple", {
|
||||
"wrapper.js": `/*!
|
||||
* First legal comment
|
||||
*/
|
||||
|
||||
/*!
|
||||
* Second legal comment
|
||||
*/
|
||||
|
||||
'use strict';
|
||||
|
||||
module.exports = require('./target');
|
||||
`,
|
||||
"target.js": `module.exports = function() { return 42; };
|
||||
`,
|
||||
"entry.js": `const fn = require('./wrapper');
|
||||
console.log(fn());
|
||||
`,
|
||||
});
|
||||
|
||||
await using proc = Bun.spawn({
|
||||
cmd: [bunExe(), "build", "entry.js", "--outfile=out.js"],
|
||||
cwd: String(dir),
|
||||
env: bunEnv,
|
||||
stdout: "pipe",
|
||||
stderr: "pipe",
|
||||
});
|
||||
|
||||
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
|
||||
|
||||
expect(exitCode).toBe(0);
|
||||
|
||||
const out = await Bun.file(`${dir}/out.js`).text();
|
||||
|
||||
// Should not contain a wrapper function
|
||||
expect(out).not.toContain("require_wrapper");
|
||||
});
|
||||
Reference in New Issue
Block a user