fix a renaming bug with external imports

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))

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,
+				},
+			},
+		},
+	})
+}

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
 		}

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 ----------

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() {