From e1b7050aa074da1a0a846a01b4a3c82857128944 Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Mon, 18 Dec 2023 16:18:09 -0500 Subject: [PATCH] fix #3319: missing symbol usage in glob transform --- CHANGELOG.md | 4 + internal/bundler_tests/bundler_glob_test.go | 55 ++++++++ .../snapshots/snapshots_glob.txt | 132 ++++++++++++++++++ internal/js_parser/js_parser.go | 1 + scripts/end-to-end-tests.js | 110 ++++++++------- 5 files changed, 248 insertions(+), 54 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 039b3228800..23cd7c7b235 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +* Fix glob imports in TypeScript files ([#3319](https://github.com/evanw/esbuild/issues/3319)) + + This release fixes a problem where bundling a TypeScript file containing a glob import could emit a call to a helper function that doesn't exist. The problem happened because esbuild's TypeScript transformation removes unused imports (which is required for correctness, as they may be type-only imports) and esbuild's glob import transformation wasn't correctly marking the imported helper function as used. This wasn't caught earlier because most of esbuild's glob import tests were written in JavaScript, not in TypeScript. + * Fix a panic when transforming optional chaining with `define` ([#3551](https://github.com/evanw/esbuild/issues/3551), [#3554](https://github.com/evanw/esbuild/pull/3554)) This release fixes a case where esbuild could crash with a panic, which was triggered by using `define` to replace an expression containing an optional chain. Here is an example: diff --git a/internal/bundler_tests/bundler_glob_test.go b/internal/bundler_tests/bundler_glob_test.go index 49dd9ebc687..dea1e410e55 100644 --- a/internal/bundler_tests/bundler_glob_test.go +++ b/internal/bundler_tests/bundler_glob_test.go @@ -37,6 +37,33 @@ func TestGlobBasicNoSplitting(t *testing.T) { }) } +func TestTSGlobBasicNoSplitting(t *testing.T) { + glob_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/entry.ts": ` + const ab = Math.random() < 0.5 ? 'a.ts' : 'b.ts' + console.log({ + concat: { + require: require('./src/' + ab), + import: import('./src/' + ab), + }, + template: { + require: require(` + "`./src/${ab}`" + `), + import: import(` + "`./src/${ab}`" + `), + }, + }) + `, + "/src/a.ts": `module.exports = 'a'`, + "/src/b.ts": `module.exports = 'b'`, + }, + entryPaths: []string{"/entry.ts"}, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputFile: "/out.js", + }, + }) +} + func TestGlobBasicSplitting(t *testing.T) { glob_suite.expectBundled(t, bundled{ files: map[string]string{ @@ -65,6 +92,34 @@ func TestGlobBasicSplitting(t *testing.T) { }) } +func TestTSGlobBasicSplitting(t *testing.T) { + glob_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/entry.ts": ` + const ab = Math.random() < 0.5 ? 'a.ts' : 'b.ts' + console.log({ + concat: { + require: require('./src/' + ab), + import: import('./src/' + ab), + }, + template: { + require: require(` + "`./src/${ab}`" + `), + import: import(` + "`./src/${ab}`" + `), + }, + }) + `, + "/src/a.ts": `module.exports = 'a'`, + "/src/b.ts": `module.exports = 'b'`, + }, + entryPaths: []string{"/entry.ts"}, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputDir: "/out", + CodeSplitting: true, + }, + }) +} + func TestGlobDirDoesNotExist(t *testing.T) { glob_suite.expectBundled(t, bundled{ files: map[string]string{ diff --git a/internal/bundler_tests/snapshots/snapshots_glob.txt b/internal/bundler_tests/snapshots/snapshots_glob.txt index ed7d1fcce74..d376aa87a55 100644 --- a/internal/bundler_tests/snapshots/snapshots_glob.txt +++ b/internal/bundler_tests/snapshots/snapshots_glob.txt @@ -258,3 +258,135 @@ console.log({ import: globImport_src_js(`./src/${ab}.js`) } }); + +================================================================================ +TestTSGlobBasicNoSplitting +---------- /out.js ---------- +// src/a.ts +var require_a = __commonJS({ + "src/a.ts"(exports, module) { + module.exports = "a"; + } +}); + +// src/b.ts +var require_b = __commonJS({ + "src/b.ts"(exports, module) { + module.exports = "b"; + } +}); + +// require("./src/**/*") in entry.ts +var globRequire_src = __glob({ + "./src/a.ts": () => require_a(), + "./src/b.ts": () => require_b() +}); + +// import("./src/**/*") in entry.ts +var globImport_src = __glob({ + "./src/a.ts": () => Promise.resolve().then(() => __toESM(require_a())), + "./src/b.ts": () => Promise.resolve().then(() => __toESM(require_b())) +}); + +// entry.ts +var ab = Math.random() < 0.5 ? "a.ts" : "b.ts"; +console.log({ + concat: { + require: globRequire_src("./src/" + ab), + import: globImport_src("./src/" + ab) + }, + template: { + require: globRequire_src(`./src/${ab}`), + import: globImport_src(`./src/${ab}`) + } +}); + +================================================================================ +TestTSGlobBasicSplitting +---------- /out/entry.js ---------- +import { + require_a +} from "./chunk-NW3BCH2J.js"; +import { + require_b +} from "./chunk-LX6LSYGO.js"; +import { + __glob +} from "./chunk-NI2FVOS3.js"; + +// require("./src/**/*") in entry.ts +var globRequire_src = __glob({ + "./src/a.ts": () => require_a(), + "./src/b.ts": () => require_b() +}); + +// import("./src/**/*") in entry.ts +var globImport_src = __glob({ + "./src/a.ts": () => import("./a-CIKLEGQA.js"), + "./src/b.ts": () => import("./b-UWTMLRMB.js") +}); + +// entry.ts +var ab = Math.random() < 0.5 ? "a.ts" : "b.ts"; +console.log({ + concat: { + require: globRequire_src("./src/" + ab), + import: globImport_src("./src/" + ab) + }, + template: { + require: globRequire_src(`./src/${ab}`), + import: globImport_src(`./src/${ab}`) + } +}); + +---------- /out/a-CIKLEGQA.js ---------- +import { + require_a +} from "./chunk-NW3BCH2J.js"; +import "./chunk-NI2FVOS3.js"; +export default require_a(); + +---------- /out/chunk-NW3BCH2J.js ---------- +import { + __commonJS +} from "./chunk-NI2FVOS3.js"; + +// src/a.ts +var require_a = __commonJS({ + "src/a.ts"(exports, module) { + module.exports = "a"; + } +}); + +export { + require_a +}; + +---------- /out/b-UWTMLRMB.js ---------- +import { + require_b +} from "./chunk-LX6LSYGO.js"; +import "./chunk-NI2FVOS3.js"; +export default require_b(); + +---------- /out/chunk-LX6LSYGO.js ---------- +import { + __commonJS +} from "./chunk-NI2FVOS3.js"; + +// src/b.ts +var require_b = __commonJS({ + "src/b.ts"(exports, module) { + module.exports = "b"; + } +}); + +export { + require_b +}; + +---------- /out/chunk-NI2FVOS3.js ---------- +export { + __glob, + __commonJS +}; diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go index 631e82d0ff1..8a56f66df57 100644 --- a/internal/js_parser/js_parser.go +++ b/internal/js_parser/js_parser.go @@ -15762,6 +15762,7 @@ outer: }) } + p.recordUsage(ref) return js_ast.Expr{Loc: expr.Loc, Data: &js_ast.ECall{ Target: js_ast.Expr{Loc: expr.Loc, Data: &js_ast.EIdentifier{Ref: ref}}, Args: []js_ast.Expr{expr}, diff --git a/scripts/end-to-end-tests.js b/scripts/end-to-end-tests.js index 175a5edf27f..0b978d9cf5e 100644 --- a/scripts/end-to-end-tests.js +++ b/scripts/end-to-end-tests.js @@ -8214,66 +8214,68 @@ if (process.platform === 'darwin' || process.platform === 'win32') { } // Test glob import behavior -tests.push( - test(['./src/*.ts', '--outdir=out', '--bundle', '--format=cjs'], { - 'node.js': ` - if (require('./out/a.js') !== 10) throw 'fail: a' - if (require('./out/b.js') !== 11) throw 'fail: b' - if (require('./out/c.js') !== 12) throw 'fail: c' - `, - 'src/a.ts': `module.exports = 10 as number`, - 'src/b.ts': `module.exports = 11 as number`, - 'src/c.ts': `module.exports = 12 as number`, - }), - test(['in.js', '--outfile=node.js', '--bundle'], { - 'in.js': ` - for (let i = 0; i < 3; i++) { - const value = require('./' + i + '.js') - if (value !== i + 10) throw 'fail: ' + i - } - `, - '0.js': `module.exports = 10`, - '1.js': `module.exports = 11`, - '2.js': `module.exports = 12`, - }), - test(['in.js', '--outfile=node.js', '--bundle'], { - 'in.js': ` - for (let i = 0; i < 3; i++) { - const value = require(\`./\${i}.js\`) - if (value !== i + 10) throw 'fail: ' + i - } - `, - '0.js': `module.exports = 10`, - '1.js': `module.exports = 11`, - '2.js': `module.exports = 12`, - }), - test(['in.js', '--outfile=node.js', '--bundle'], { - 'in.js': ` - export let async = async () => { +for (const ext of ['.js', '.ts']) { + tests.push( + test(['./src/*' + ext, '--outdir=out', '--bundle', '--format=cjs'], { + 'node.js': ` + if (require('./out/a.js') !== 10) throw 'fail: a' + if (require('./out/b.js') !== 11) throw 'fail: b' + if (require('./out/c.js') !== 12) throw 'fail: c' + `, + ['src/a' + ext]: `module.exports = 10`, + ['src/b' + ext]: `module.exports = 11`, + ['src/c' + ext]: `module.exports = 12`, + }), + test(['in' + ext, '--outfile=node.js', '--bundle'], { + ['in' + ext]: ` for (let i = 0; i < 3; i++) { - const { default: value } = await import('./' + i + '.js') + const value = require('./' + i + '${ext}') if (value !== i + 10) throw 'fail: ' + i } - } - `, - '0.js': `export default 10`, - '1.js': `export default 11`, - '2.js': `export default 12`, - }, { async: true }), - test(['in.js', '--outfile=node.js', '--bundle'], { - 'in.js': ` - export let async = async () => { + `, + ['0' + ext]: `module.exports = 10`, + ['1' + ext]: `module.exports = 11`, + ['2' + ext]: `module.exports = 12`, + }), + test(['in' + ext, '--outfile=node.js', '--bundle'], { + ['in' + ext]: ` for (let i = 0; i < 3; i++) { - const { default: value } = await import(\`./\${i}.js\`) + const value = require(\`./\${i}${ext}\`) if (value !== i + 10) throw 'fail: ' + i } - } - `, - '0.js': `export default 10`, - '1.js': `export default 11`, - '2.js': `export default 12`, - }, { async: true }), -) + `, + ['0' + ext]: `module.exports = 10`, + ['1' + ext]: `module.exports = 11`, + ['2' + ext]: `module.exports = 12`, + }), + test(['in' + ext, '--outfile=node.js', '--bundle'], { + ['in' + ext]: ` + export let async = async () => { + for (let i = 0; i < 3; i++) { + const { default: value } = await import('./' + i + '${ext}') + if (value !== i + 10) throw 'fail: ' + i + } + } + `, + ['0' + ext]: `export default 10`, + ['1' + ext]: `export default 11`, + ['2' + ext]: `export default 12`, + }, { async: true }), + test(['in' + ext, '--outfile=node.js', '--bundle'], { + ['in' + ext]: ` + export let async = async () => { + for (let i = 0; i < 3; i++) { + const { default: value } = await import(\`./\${i}${ext}\`) + if (value !== i + 10) throw 'fail: ' + i + } + } + `, + ['0' + ext]: `export default 10`, + ['1' + ext]: `export default 11`, + ['2' + ext]: `export default 12`, + }, { async: true }), + ) +} // Test "using" declarations for (const flags of [[], '--supported:async-await=false']) {