track and retain intermediate re-exporting files

CHANGELOG.md

@@ -8,6 +8,16 @@
 
     With this release esbuild will now attempt to define `process.env.NODE_ENV` automatically instead of warning about it. This will be implicitly defined to `"production"` if minification is enabled and `"development"` otherwise. This automatic behavior only happens when the platform is `browser`, since `process` is not a valid browser API and will never exist in the browser. This is also only done if there are no existing defines for `process`, `process.env`, or `process.env.NODE_ENV` so you can override the automatic value if necessary. If you need to disable this behavior, you can use the `neutral` platform instead of the `browser` platform.
 
+* Retain side-effect free intermediate re-exporting files ([#1088](https://github.com/evanw/esbuild/issues/1088))
+
+    This fixes a subtle bug with esbuild's support for [Webpack's `"sideEffects": false` annotation](https://webpack.js.org/guides/tree-shaking/#mark-the-file-as-side-effect-free) in `package.json` when combined with re-export statements. A re-export is when you import something from one file and then export it again. You can re-export something with `export * from` or `export {foo} from` or `import {foo} from` followed by `export {foo}`.
+
+    The bug was that files which only contain re-exports and that are marked as being side-effect free were not being included in the bundle if you import one of the re-exported symbols. This is because esbuild's implementation of re-export linking caused the original importing file to "short circuit" the re-export and just import straight from the file containing the final symbol, skipping the file containing the re-export entirely.
+
+    This was normally not observable since the intermediate file consisted entirely of re-exports, which have no side effects. However, a recent change to allow ESM files to be lazily-initialized relies on all intermediate files being included in the bundle to trigger the initialization of the lazy evaluation wrappers. So the behavior of skipping over re-export files is now causing the imported symbols to not be initialized if the re-exported file is marked as lazily-evaluated.
+
+    The fix is to track all re-exports in the import chain from the original file to the file containing the final symbol and then retain all of those statements if the import ends up being used.
+
 ## 0.11.2
 
 * Fix missing symbol dependency for wrapped ESM files ([#1086](https://github.com/evanw/esbuild/issues/1086))

internal/bundler/bundler_dce_test.go

@@ -808,6 +808,240 @@ func TestPackageJsonSideEffectsFalseNoWarningInNodeModulesIssue999(t *testing.T)
 	})
 }
 
+func TestPackageJsonSideEffectsFalseIntermediateFilesUnused(t *testing.T) {
+	dce_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/Users/user/project/src/entry.js": `
+				import {foo} from "demo-pkg"
+			`,
+			"/Users/user/project/node_modules/demo-pkg/index.js": `
+				export {foo} from "./foo.js"
+				throw 'REMOVE THIS'
+			`,
+			"/Users/user/project/node_modules/demo-pkg/foo.js": `
+				export const foo = 123
+			`,
+			"/Users/user/project/node_modules/demo-pkg/package.json": `
+				{ "sideEffects": false }
+			`,
+		},
+		entryPaths: []string{"/Users/user/project/src/entry.js"},
+		options: config.Options{
+			Mode:          config.ModeBundle,
+			AbsOutputFile: "/out.js",
+		},
+	})
+}
+
+func TestPackageJsonSideEffectsFalseIntermediateFilesUsed(t *testing.T) {
+	dce_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/Users/user/project/src/entry.js": `
+				import {foo} from "demo-pkg"
+				console.log(foo)
+			`,
+			"/Users/user/project/node_modules/demo-pkg/index.js": `
+				export {foo} from "./foo.js"
+				throw 'keep this'
+			`,
+			"/Users/user/project/node_modules/demo-pkg/foo.js": `
+				export const foo = 123
+			`,
+			"/Users/user/project/node_modules/demo-pkg/package.json": `
+				{ "sideEffects": false }
+			`,
+		},
+		entryPaths: []string{"/Users/user/project/src/entry.js"},
+		options: config.Options{
+			Mode:          config.ModeBundle,
+			AbsOutputFile: "/out.js",
+		},
+	})
+}
+
+func TestPackageJsonSideEffectsFalseIntermediateFilesChainAll(t *testing.T) {
+	dce_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/Users/user/project/src/entry.js": `
+				import {foo} from "a"
+				console.log(foo)
+			`,
+			"/Users/user/project/node_modules/a/index.js": `
+				export {foo} from "b"
+			`,
+			"/Users/user/project/node_modules/a/package.json": `
+				{ "sideEffects": false }
+			`,
+			"/Users/user/project/node_modules/b/index.js": `
+				export {foo} from "c"
+				throw 'keep this'
+			`,
+			"/Users/user/project/node_modules/b/package.json": `
+				{ "sideEffects": false }
+			`,
+			"/Users/user/project/node_modules/c/index.js": `
+				export {foo} from "d"
+			`,
+			"/Users/user/project/node_modules/c/package.json": `
+				{ "sideEffects": false }
+			`,
+			"/Users/user/project/node_modules/d/index.js": `
+				export const foo = 123
+			`,
+			"/Users/user/project/node_modules/d/package.json": `
+				{ "sideEffects": false }
+			`,
+		},
+		entryPaths: []string{"/Users/user/project/src/entry.js"},
+		options: config.Options{
+			Mode:          config.ModeBundle,
+			AbsOutputFile: "/out.js",
+		},
+	})
+}
+
+func TestPackageJsonSideEffectsFalseIntermediateFilesChainOne(t *testing.T) {
+	dce_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/Users/user/project/src/entry.js": `
+				import {foo} from "a"
+				console.log(foo)
+			`,
+			"/Users/user/project/node_modules/a/index.js": `
+				export {foo} from "b"
+			`,
+			"/Users/user/project/node_modules/b/index.js": `
+				export {foo} from "c"
+				throw 'keep this'
+			`,
+			"/Users/user/project/node_modules/b/package.json": `
+				{ "sideEffects": false }
+			`,
+			"/Users/user/project/node_modules/c/index.js": `
+				export {foo} from "d"
+			`,
+			"/Users/user/project/node_modules/d/index.js": `
+				export const foo = 123
+			`,
+		},
+		entryPaths: []string{"/Users/user/project/src/entry.js"},
+		options: config.Options{
+			Mode:          config.ModeBundle,
+			AbsOutputFile: "/out.js",
+		},
+	})
+}
+
+func TestPackageJsonSideEffectsFalseIntermediateFilesDiamond(t *testing.T) {
+	dce_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/Users/user/project/src/entry.js": `
+				import {foo} from "a"
+				console.log(foo)
+			`,
+			"/Users/user/project/node_modules/a/index.js": `
+				export * from "b1"
+				export * from "b2"
+			`,
+			"/Users/user/project/node_modules/b1/index.js": `
+				export {foo} from "c"
+				throw 'keep this 1'
+			`,
+			"/Users/user/project/node_modules/b1/package.json": `
+				{ "sideEffects": false }
+			`,
+			"/Users/user/project/node_modules/b2/index.js": `
+				export {foo} from "c"
+				throw 'keep this 2'
+			`,
+			"/Users/user/project/node_modules/b2/package.json": `
+				{ "sideEffects": false }
+			`,
+			"/Users/user/project/node_modules/c/index.js": `
+				export {foo} from "d"
+			`,
+			"/Users/user/project/node_modules/d/index.js": `
+				export const foo = 123
+			`,
+		},
+		entryPaths: []string{"/Users/user/project/src/entry.js"},
+		options: config.Options{
+			Mode:          config.ModeBundle,
+			AbsOutputFile: "/out.js",
+		},
+	})
+}
+
+func TestPackageJsonSideEffectsFalseOneFork(t *testing.T) {
+	dce_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/Users/user/project/src/entry.js": `
+				import("a").then(x => assert(x.foo === "foo"))
+			`,
+			"/Users/user/project/node_modules/a/index.js": `
+				export {foo} from "b"
+			`,
+			"/Users/user/project/node_modules/b/index.js": `
+				export {foo, bar} from "c"
+				export {baz} from "d"
+			`,
+			"/Users/user/project/node_modules/b/package.json": `
+				{ "sideEffects": false }
+			`,
+			"/Users/user/project/node_modules/c/index.js": `
+				export let foo = "foo"
+				export let bar = "bar"
+			`,
+			"/Users/user/project/node_modules/d/index.js": `
+				export let baz = "baz"
+			`,
+		},
+		entryPaths: []string{"/Users/user/project/src/entry.js"},
+		options: config.Options{
+			Mode:          config.ModeBundle,
+			AbsOutputFile: "/out.js",
+		},
+	})
+}
+
+func TestPackageJsonSideEffectsFalseAllFork(t *testing.T) {
+	dce_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/Users/user/project/src/entry.js": `
+				import("a").then(x => assert(x.foo === "foo"))
+			`,
+			"/Users/user/project/node_modules/a/index.js": `
+				export {foo} from "b"
+			`,
+			"/Users/user/project/node_modules/b/index.js": `
+				export {foo, bar} from "c"
+				export {baz} from "d"
+			`,
+			"/Users/user/project/node_modules/b/package.json": `
+				{ "sideEffects": false }
+			`,
+			"/Users/user/project/node_modules/c/index.js": `
+				export let foo = "foo"
+				export let bar = "bar"
+			`,
+			"/Users/user/project/node_modules/c/package.json": `
+				{ "sideEffects": false }
+			`,
+			"/Users/user/project/node_modules/d/index.js": `
+				export let baz = "baz"
+			`,
+			"/Users/user/project/node_modules/d/package.json": `
+				{ "sideEffects": false }
+			`,
+		},
+		entryPaths: []string{"/Users/user/project/src/entry.js"},
+		options: config.Options{
+			Mode:          config.ModeBundle,
+			AbsOutputFile: "/out.js",
+		},
+	})
+}
+
 func TestJSONLoaderRemoveUnused(t *testing.T) {
 	dce_suite.expectBundled(t, bundled{
 		files: map[string]string{

internal/bundler/linker.go

@@ -232,6 +232,14 @@ type jsMeta struct {
 }
 
 type importData struct {
+	// This is an array of intermediate statements that re-exported this symbol
+	// in a chain before getting to the final symbol. This can be done either with
+	// "export * from" or "export {} from". If this is done with "export * from"
+	// then this may not be the result of a single chain but may instead form
+	// a diamond shape if this same symbol was re-exported multiple times from
+	// different files.
+	reExports []nonLocalDependency
+
 	sourceIndex uint32
 	nameLoc     logger.Loc // Optional, goes with sourceIndex, ignore if zero
 	ref         js_ast.Ref
@@ -1600,12 +1608,17 @@ func (c *linkerContext) scanImportsAndExports() {
 			for _, partIndex := range repr.ast.NamedImports[importRef].LocalPartsWithUses {
 				partMeta := &repr.meta.partMeta[partIndex]
 
+				// Depend on the file containing the imported symbol
 				for _, resolvedPartIndex := range partsDeclaringSymbol {
 					partMeta.nonLocalDependencies = append(partMeta.nonLocalDependencies, nonLocalDependency{
 						sourceIndex: importData.sourceIndex,
 						partIndex:   resolvedPartIndex,
 					})
 				}
+
+				// Also depend on any files that re-exported this symbol in between the
+				// file containing the import and the file containing the imported symbol
+				partMeta.nonLocalDependencies = append(partMeta.nonLocalDependencies, importData.reExports...)
 			}
 
 			// Merge these symbols so they will share the same name
@@ -1740,6 +1753,7 @@ func (c *linkerContext) createExportsForFile(sourceIndex uint32) {
 		if importData, ok := c.files[export.sourceIndex].repr.(*reprJS).meta.importsToBind[export.ref]; ok {
 			export.ref = importData.ref
 			export.sourceIndex = importData.sourceIndex
+			nsExportNonLocalDependencies = append(nsExportNonLocalDependencies, importData.reExports...)
 		}
 
 		// Exports of imports need EImportIdentifier in case they need to be re-
@@ -1892,12 +1906,13 @@ func (c *linkerContext) matchImportsWithExportsForFile(sourceIndex uint32) {
 		c.cycleDetector = c.cycleDetector[:0]
 
 		importRef := js_ast.Ref{OuterIndex: sourceIndex, InnerIndex: uint32(innerIndex)}
-		result := c.matchImportWithExport(importTracker{sourceIndex: sourceIndex, importRef: importRef})
+		result, reExports := c.matchImportWithExport(importTracker{sourceIndex: sourceIndex, importRef: importRef}, nil)
 		switch result.kind {
 		case matchImportIgnore:
 
 		case matchImportNormal:
 			repr.meta.importsToBind[importRef] = importData{
+				reExports:   reExports,
 				sourceIndex: result.sourceIndex,
 				ref:         result.ref,
 			}
@@ -1910,6 +1925,7 @@ func (c *linkerContext) matchImportsWithExportsForFile(sourceIndex uint32) {
 
 		case matchImportNormalAndNamespace:
 			repr.meta.importsToBind[importRef] = importData{
+				reExports:   reExports,
 				sourceIndex: result.sourceIndex,
 				ref:         result.ref,
 			}
@@ -1998,8 +2014,11 @@ type matchImportResult struct {
 	ref              js_ast.Ref
 }
 
-func (c *linkerContext) matchImportWithExport(tracker importTracker) (result matchImportResult) {
+func (c *linkerContext) matchImportWithExport(
+	tracker importTracker, reExportsIn []nonLocalDependency,
+) (result matchImportResult, reExports []nonLocalDependency) {
 	var ambiguousResults []matchImportResult
+	reExports = reExportsIn
 
 loop:
 	for {
@@ -2094,7 +2113,8 @@ loop:
 				// time, so we emit a warning and rewrite the value to the literal
 				// "undefined" instead of emitting an error.
 				symbol.ImportItemStatus = js_ast.ImportItemMissing
-				c.log.AddRangeWarning(&source, r, fmt.Sprintf("Import %q will always be undefined because there is no matching export", namedImport.Alias))
+				c.log.AddRangeWarning(&source, r, fmt.Sprintf(
+					"Import %q will always be undefined because there is no matching export", namedImport.Alias))
 			} else {
 				c.log.AddRangeError(&source, r, fmt.Sprintf("No matching export in %q for import %q",
 					c.files[nextTracker.sourceIndex].source.PrettyPath, namedImport.Alias))
@@ -2111,10 +2131,15 @@ loop:
 			for _, ambiguousTracker := range potentiallyAmbiguousExportStarRefs {
 				// If this is a re-export of another import, follow the import
 				if _, ok := c.files[ambiguousTracker.sourceIndex].repr.(*reprJS).ast.NamedImports[ambiguousTracker.ref]; ok {
-					ambiguousResults = append(ambiguousResults, c.matchImportWithExport(importTracker{
+					// Save and restore the cycle detector to avoid mixing information
+					oldCycleDetector := c.cycleDetector
+					ambiguousResult, newReExportFiles := c.matchImportWithExport(importTracker{
 						sourceIndex: ambiguousTracker.sourceIndex,
 						importRef:   ambiguousTracker.ref,
-					}))
+					}, reExports)
+					c.cycleDetector = oldCycleDetector
+					ambiguousResults = append(ambiguousResults, ambiguousResult)
+					reExports = newReExportFiles
 				} else {
 					ambiguousResults = append(ambiguousResults, matchImportResult{
 						kind:        matchImportNormal,
@@ -2137,6 +2162,15 @@ loop:
 				nameLoc:     nextTracker.nameLoc,
 			}
 
+			// Depend on the statement(s) that declared this import symbol in the
+			// original file
+			for _, resolvedPartIndex := range c.files[tracker.sourceIndex].repr.(*reprJS).ast.TopLevelSymbolToParts[tracker.importRef] {
+				reExports = append(reExports, nonLocalDependency{
+					sourceIndex: tracker.sourceIndex,
+					partIndex:   resolvedPartIndex,
+				})
+			}
+
 			// If this is a re-export of another import, continue for another
 			// iteration of the loop to resolve that import as well
 			if _, ok := c.files[nextTracker.sourceIndex].repr.(*reprJS).ast.NamedImports[nextTracker.importRef]; ok {
@@ -2163,9 +2197,9 @@ loop:
 					nameLoc:          result.nameLoc,
 					otherSourceIndex: ambiguousResult.sourceIndex,
 					otherNameLoc:     ambiguousResult.nameLoc,
-				}
+				}, nil
 			}
-			return matchImportResult{kind: matchImportAmbiguous}
+			return matchImportResult{kind: matchImportAmbiguous}, nil
 		}
 	}