diff --git a/src/ast/Parser.zig b/src/ast/Parser.zig index bc882d7ac1..fdb60c1ab9 100644 --- a/src/ast/Parser.zig +++ b/src/ast/Parser.zig @@ -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 = .{}; diff --git a/test/regression/issue/3179.test.ts b/test/regression/issue/3179.test.ts new file mode 100644 index 0000000000..70400f23ac --- /dev/null +++ b/test/regression/issue/3179.test.ts @@ -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"); +});