From cf870286fe799ff436474908a0ce4544ea47a37b Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Thu, 1 Apr 2021 00:18:03 -0700 Subject: [PATCH] track and retain intermediate re-exporting files --- internal/bundler/bundler_dce_test.go | 234 +++++++++++++++++++ internal/bundler/linker.go | 48 +++- internal/bundler/snapshots/snapshots_dce.txt | 112 +++++++++ scripts/end-to-end-tests.js | 39 ++++ 4 files changed, 426 insertions(+), 7 deletions(-) diff --git a/internal/bundler/bundler_dce_test.go b/internal/bundler/bundler_dce_test.go index ec2778563e6..7066822420a 100644 --- a/internal/bundler/bundler_dce_test.go +++ b/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{ diff --git a/internal/bundler/linker.go b/internal/bundler/linker.go index 8d5cc707684..49b3216c0c4 100644 --- a/internal/bundler/linker.go +++ b/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 } } diff --git a/internal/bundler/snapshots/snapshots_dce.txt b/internal/bundler/snapshots/snapshots_dce.txt index 3e1051f0af1..4c510b7d756 100644 --- a/internal/bundler/snapshots/snapshots_dce.txt +++ b/internal/bundler/snapshots/snapshots_dce.txt @@ -232,6 +232,87 @@ TestPackageJsonSideEffectsArrayRemove // Users/user/project/src/entry.js console.log("unused import"); +================================================================================ +TestPackageJsonSideEffectsFalseAllFork +---------- /out.js ---------- +// Users/user/project/node_modules/c/index.js +var foo; +var init_c = __esm(() => { + foo = "foo"; +}); + +// Users/user/project/node_modules/b/index.js +var init_b = __esm(() => { + init_c(); +}); + +// Users/user/project/node_modules/a/index.js +var a_exports = {}; +__export(a_exports, { + foo: () => foo +}); +var init_a = __esm(() => { + init_b(); +}); + +// Users/user/project/src/entry.js +Promise.resolve().then(() => (init_a(), a_exports)).then((x) => assert(x.foo === "foo")); + +================================================================================ +TestPackageJsonSideEffectsFalseIntermediateFilesChainAll +---------- /out.js ---------- +// Users/user/project/node_modules/d/index.js +var foo = 123; + +// Users/user/project/node_modules/b/index.js +throw "keep this"; + +// Users/user/project/src/entry.js +console.log(foo); + +================================================================================ +TestPackageJsonSideEffectsFalseIntermediateFilesChainOne +---------- /out.js ---------- +// Users/user/project/node_modules/d/index.js +var foo = 123; + +// Users/user/project/node_modules/b/index.js +throw "keep this"; + +// Users/user/project/src/entry.js +console.log(foo); + +================================================================================ +TestPackageJsonSideEffectsFalseIntermediateFilesDiamond +---------- /out.js ---------- +// Users/user/project/node_modules/d/index.js +var foo = 123; + +// Users/user/project/node_modules/b1/index.js +throw "keep this 1"; + +// Users/user/project/node_modules/b2/index.js +throw "keep this 2"; + +// Users/user/project/src/entry.js +console.log(foo); + +================================================================================ +TestPackageJsonSideEffectsFalseIntermediateFilesUnused +---------- /out.js ---------- + +================================================================================ +TestPackageJsonSideEffectsFalseIntermediateFilesUsed +---------- /out.js ---------- +// Users/user/project/node_modules/demo-pkg/foo.js +var foo = 123; + +// Users/user/project/node_modules/demo-pkg/index.js +throw "keep this"; + +// Users/user/project/src/entry.js +console.log(foo); + ================================================================================ TestPackageJsonSideEffectsFalseKeepBareImportAndRequireCommonJS ---------- /out.js ---------- @@ -322,6 +403,37 @@ console.log("unused import"); // Users/user/project/src/entry.js console.log("used import"); +================================================================================ +TestPackageJsonSideEffectsFalseOneFork +---------- /out.js ---------- +// Users/user/project/node_modules/c/index.js +var foo; +var init_c = __esm(() => { + foo = "foo"; +}); + +// Users/user/project/node_modules/d/index.js +var init_d = __esm(() => { +}); + +// Users/user/project/node_modules/b/index.js +var init_b = __esm(() => { + init_c(); + init_d(); +}); + +// Users/user/project/node_modules/a/index.js +var a_exports = {}; +__export(a_exports, { + foo: () => foo +}); +var init_a = __esm(() => { + init_b(); +}); + +// Users/user/project/src/entry.js +Promise.resolve().then(() => (init_a(), a_exports)).then((x) => assert(x.foo === "foo")); + ================================================================================ TestPackageJsonSideEffectsFalseRemoveBareImportCommonJS ---------- /out.js ---------- diff --git a/scripts/end-to-end-tests.js b/scripts/end-to-end-tests.js index 2898a2a0737..c12a131de11 100644 --- a/scripts/end-to-end-tests.js +++ b/scripts/end-to-end-tests.js @@ -1502,6 +1502,45 @@ }), ) + // Check for "sideEffects: false" wrapper handling + // https://github.com/evanw/esbuild/issues/1088 + for (const pkgJSON of [`{}`, `{"sideEffects": false}`]) { + for (const entry of [ + `export let async = async () => { if (require("pkg").foo() !== 123) throw 'fail' }`, + `export let async = () => import("pkg").then(x => { if (x.foo() !== 123) throw 'fail' })`, + ]) { + for (const index of [`export {foo} from "./foo.js"`, `import {foo} from "./foo.js"; export {foo}`]) { + for (const foo of [`export let foo = () => 123`, `exports.foo = () => 123`]) { + tests.push(test(['in.js', '--outfile=node.js', '--bundle'], { + 'in.js': entry, + 'node_modules/pkg/package.json': pkgJSON, + 'node_modules/pkg/index.js': index, + 'node_modules/pkg/foo.js': foo, + }, { async: true })) + } + } + } + for (const entry of [ + `export let async = async () => { try { require("pkg") } catch (e) { return } throw 'fail' }`, + `export let async = () => import("pkg").then(x => { throw 'fail' }, () => {})`, + ]) { + tests.push(test(['in.js', '--outfile=node.js', '--bundle'], { + 'in.js': entry, + 'node_modules/pkg/package.json': pkgJSON, + 'node_modules/pkg/index.js': ` + export {foo} from './b.js' + `, + 'node_modules/pkg/b.js': ` + export {foo} from './c.js' + throw 'stop' + `, + 'node_modules/pkg/c.js': ` + export let foo = () => 123 + `, + }, { async: true })) + } + } + // Tests for "arguments" scope issues tests.push( test(['in.js', '--outfile=node.js', '--minify'], {