From 1f3bd9d1834c5e411b2a9ad74dfa683cbb8b81dd Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Sun, 29 Nov 2020 20:47:26 -0800 Subject: [PATCH] fix a renaming bug with external imports --- CHANGELOG.md | 46 ++++++++++++++++++ internal/bundler/bundler_default_test.go | 42 ++++++++++++++++ internal/bundler/linker.go | 39 +++++++++++++++ .../bundler/snapshots/snapshots_default.txt | 48 +++++++++++++++++++ scripts/plugin-tests.js | 34 +++++++++++++ 5 files changed, 209 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4361e8656ae..2f35ce1ac27 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,51 @@ # Changelog +## Unreleased + +* Fix a renaming bug with external imports + + There was a possibility of a cross-module name collision while bundling in a certain edge case. Specifically, when multiple files both contained an `import` statement to an external module and then both of those files were imported using `require`. For example: + + ```js + // index.js + console.log(require('./a.js'), require('./b.js')) + ``` + + ```js + // a.js + export {exists} from 'fs' + ``` + + ```js + // b.js + export {exists} from 'fs' + ``` + + In this case the files `a.js` and `b.js` are converted to CommonJS format so they can be imported using `require`: + + ```js + // a.js + import {exists} from "fs"; + var require_a = __commonJS((exports) => { + __export(exports, { + exists: () => exists + }); + }); + + // b.js + import {exists} from "fs"; + var require_b = __commonJS((exports) => { + __export(exports, { + exists: () => exists + }); + }); + + // index.js + console.log(require_a(), require_b()); + ``` + + However, the `exists` symbol has been duplicated without being renamed. This is will result in a syntax error at run-time. The reason this happens is that the statements in the files `a.js` and `b.js` are placed in a nested scope because they are inside the CommonJS closure. The `import` statements were extracted outside the closure but the symbols they declared were incorrectly not added to the outer scope. This problem has been fixed, and this edge case should no longer result in name collisions. + ## 0.8.17 * Get esbuild working on the Apple M1 chip via Rosetta 2 ([#564](https://github.com/evanw/esbuild/pull/564)) diff --git a/internal/bundler/bundler_default_test.go b/internal/bundler/bundler_default_test.go index d058ad6b591..77511230283 100644 --- a/internal/bundler/bundler_default_test.go +++ b/internal/bundler/bundler_default_test.go @@ -3721,3 +3721,45 @@ func TestRequireMainIIFE(t *testing.T) { `, }) } + +func TestExternalES6ConvertedToCommonJS(t *testing.T) { + default_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/entry.js": ` + require('./a') + require('./b') + require('./c') + require('./d') + require('./e') + `, + "/a.js": ` + import * as ns from 'x' + export {ns} + `, + "/b.js": ` + import * as ns from 'x' // "ns" must be renamed to avoid collisions with "a.js" + export {ns} + `, + "/c.js": ` + export * as ns from 'x' + `, + "/d.js": ` + export {ns} from 'x' + `, + "/e.js": ` + export * from 'x' + `, + }, + entryPaths: []string{"/entry.js"}, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputFile: "/out.js", + OutputFormat: config.FormatESModule, + ExternalModules: config.ExternalModules{ + NodeModules: map[string]bool{ + "x": true, + }, + }, + }, + }) +} diff --git a/internal/bundler/linker.go b/internal/bundler/linker.go index 404defe6066..285837144a3 100644 --- a/internal/bundler/linker.go +++ b/internal/bundler/linker.go @@ -3245,6 +3245,45 @@ func (c *linkerContext) renameSymbolsInChunk(chunk *chunkInfo, filesInOrder []ui // renaming code. if repr.meta.cjsWrap { r.AddTopLevelSymbol(repr.ast.WrapperRef) + + // External import statements will be hoisted outside of the CommonJS + // wrapper if the output format supports import statements. We need to + // add those symbols to the top-level scope to avoid causing name + // collisions. This code special-cases only those symbols. + if c.options.OutputFormat.KeepES6ImportExportSyntax() { + for _, part := range repr.ast.Parts { + for _, stmt := range part.Stmts { + switch s := stmt.Data.(type) { + case *js_ast.SImport: + if repr.ast.ImportRecords[s.ImportRecordIndex].SourceIndex == nil { + r.AddTopLevelSymbol(s.NamespaceRef) + if s.DefaultName != nil { + r.AddTopLevelSymbol(s.DefaultName.Ref) + } + if s.Items != nil { + for _, item := range *s.Items { + r.AddTopLevelSymbol(item.Name.Ref) + } + } + } + + case *js_ast.SExportStar: + if repr.ast.ImportRecords[s.ImportRecordIndex].SourceIndex == nil { + r.AddTopLevelSymbol(s.NamespaceRef) + } + + case *js_ast.SExportFrom: + if repr.ast.ImportRecords[s.ImportRecordIndex].SourceIndex == nil { + r.AddTopLevelSymbol(s.NamespaceRef) + for _, item := range s.Items { + r.AddTopLevelSymbol(item.Name.Ref) + } + } + } + } + } + } + nestedScopes[sourceIndex] = []*js_ast.Scope{repr.ast.ModuleScope} continue } diff --git a/internal/bundler/snapshots/snapshots_default.txt b/internal/bundler/snapshots/snapshots_default.txt index ef4ab77e8a3..36f805381cd 100644 --- a/internal/bundler/snapshots/snapshots_default.txt +++ b/internal/bundler/snapshots/snapshots_default.txt @@ -689,6 +689,54 @@ var bar = 123; // /entry.js console.log(exports, module.exports, test_exports, test_exports2); +================================================================================ +TestExternalES6ConvertedToCommonJS +---------- /out.js ---------- +// /a.js +import * as ns from "x"; +var require_a = __commonJS((exports) => { + __export(exports, { + ns: () => ns + }); +}); + +// /b.js +import * as ns2 from "x"; +var require_b = __commonJS((exports) => { + __export(exports, { + ns: () => ns2 + }); +}); + +// /c.js +import * as ns3 from "x"; +var require_c = __commonJS((exports) => { + __export(exports, { + ns: () => ns3 + }); +}); + +// /d.js +import {ns as ns4} from "x"; +var require_d = __commonJS((exports) => { + __export(exports, { + ns: () => ns4 + }); +}); + +// /e.js +import * as x_star from "x"; +var require_e = __commonJS((exports) => { + __exportStar(exports, x_star); +}); + +// /entry.js +require_a(); +require_b(); +require_c(); +require_d(); +require_e(); + ================================================================================ TestExternalModuleExclusionPackage ---------- /out.js ---------- diff --git a/scripts/plugin-tests.js b/scripts/plugin-tests.js index 08dc91907b0..4345cccddbe 100644 --- a/scripts/plugin-tests.js +++ b/scripts/plugin-tests.js @@ -607,6 +607,40 @@ let pluginTests = { const result = require(output) assert.strictEqual(result.default, 123) }, + + async externalRequire({ esbuild, testDir }) { + const externalPlugin = external => ({ + name: 'external', + setup(build) { + let escape = text => `^${text.replace(/[-\/\\^$*+?.()|[\]{}]/g, '\\$&')}$` + let filter = new RegExp(external.map(escape).join('|')) + build.onResolve({ filter: /.*/, namespace: 'external' }, args => ({ + path: args.path, external: true + })) + build.onResolve({ filter }, args => ({ + path: args.path, namespace: 'external' + })) + build.onLoad({ filter: /.*/, namespace: 'external' }, args => ({ + contents: `import * as all from ${JSON.stringify(args.path)}; module.exports = all` + })) + }, + }) + const outfile = path.join(testDir, 'out', 'output.mjs') + await esbuild.build({ + stdin: { + contents: ` + const fs = require('fs') + const path = require('path') + export default fs.readdirSync(path.dirname(new URL(import.meta.url).pathname)) + `, + }, + bundle: true, outfile, format: 'esm', plugins: [ + externalPlugin(['fs', 'path']) + ], + }) + const result = await import(outfile) + assert.deepStrictEqual(result.default, [path.basename(outfile)]) + }, } async function main() {