separate "ignore annotations" from "tree shaking" (#1625)

cmd/esbuild/main.go

@@ -72,6 +72,8 @@ var helpText = func(colors logger.Colors) string {
   --footer:T=...            Text to be appended to each output file of type T
                             where T is one of: css | js
   --global-name=...         The name of the global for the IIFE format
+  --ignore-annotations      Enable this to work with packages that have
+                            incorrect tree-shaking annotations
   --inject:F                Import the file F into all input files and
                             automatically replace matching globals with imports
   --jsx-factory=...         What to use for JSX instead of React.createElement
@@ -105,8 +107,7 @@ var helpText = func(colors logger.Colors) string {
   --sourcemap=external      Do not link to the source map with a comment
   --sourcemap=inline        Emit the source map with an inline data URL
   --sources-content=false   Omit "sourcesContent" in generated source maps
-  --tree-shaking=...        Set to "ignore-annotations" to work with packages
-                            that have incorrect tree-shaking annotations
+  --tree-shaking=...        Force tree shaking on or off (false | true)
   --tsconfig=...            Use this tsconfig.json file instead of other ones
   --version                 Print the current version (` + esbuildVersion + `) and exit
 

internal/bundler/bundler.go

@@ -2310,7 +2310,7 @@ func (cache *runtimeCache) parseRuntime(options *config.Options) (source logger.
 
 		// Always do tree shaking for the runtime because we never want to
 		// include unnecessary runtime code
-		Mode: config.ModeBundle,
+		TreeShaking: true,
 	}))
 	if log.HasErrors() {
 		msgs := "Internal error: failed to parse runtime:\n"

internal/bundler/bundler_default_test.go

@@ -3786,6 +3786,7 @@ func TestInjectNoBundle(t *testing.T) {
 		entryPaths: []string{"/entry.js"},
 		options: config.Options{
 			Mode:          config.ModePassThrough,
+			TreeShaking:   true,
 			AbsOutputFile: "/out.js",
 			Defines:       &defines,
 			InjectAbsPaths: []string{

internal/bundler/bundler_test.go

@@ -77,6 +77,10 @@ func (s *suite) expectBundled(t *testing.T, args bundled) {
 		if args.options.AbsOutputFile != "" {
 			args.options.AbsOutputDir = path.Dir(args.options.AbsOutputFile)
 		}
+		if args.options.Mode == config.ModeBundle || (args.options.Mode == config.ModeConvertFormat && args.options.OutputFormat == config.FormatIIFE) {
+			// Apply this default to all tests since it was not configurable when the tests were written
+			args.options.TreeShaking = true
+		}
 		log := logger.NewDeferLog(logger.DeferLogNoVerboseOrDebug)
 		caches := cache.MakeCacheSet()
 		resolver := resolver.NewResolver(fs, log, caches, args.options)

internal/bundler/linker.go

@@ -2571,8 +2571,6 @@ func (c *linkerContext) markFileLiveForTreeShaking(sourceIndex uint32) {
 
 	switch repr := file.InputFile.Repr.(type) {
 	case *graph.JSRepr:
-		isTreeShakingEnabled := config.IsTreeShakingEnabled(c.options.Mode, c.options.OutputFormat)
-
 		// If the JavaScript stub for a CSS file is included, also include the CSS file
 		if repr.CSSSourceIndex.IsValid() {
 			c.markFileLiveForTreeShaking(repr.CSSSourceIndex.GetIndex())
@@ -2609,7 +2607,7 @@ func (c *linkerContext) markFileLiveForTreeShaking(sourceIndex uint32) {
 			// Include all parts in this file with side effects, or just include
 			// everything if tree-shaking is disabled. Note that we still want to
 			// perform tree-shaking on the runtime even if tree-shaking is disabled.
-			if !canBeRemovedIfUnused || (!part.ForceTreeShaking && !isTreeShakingEnabled && file.IsEntryPoint()) {
+			if !canBeRemovedIfUnused || (!part.ForceTreeShaking && !c.options.TreeShaking && file.IsEntryPoint()) {
 				c.markPartLiveForTreeShaking(sourceIndex, uint32(partIndex))
 			}
 		}

internal/config/config.go

@@ -221,6 +221,7 @@ type Options struct {
 	ASCIIOnly               bool
 	KeepNames               bool
 	IgnoreDCEAnnotations    bool
+	TreeShaking             bool
 
 	Defines  *ProcessedDefines
 	TS       TSOptions
@@ -384,10 +385,6 @@ func SubstituteTemplate(template []PathTemplate, placeholders PathPlaceholders)
 	return result
 }
 
-func IsTreeShakingEnabled(mode Mode, outputFormat Format) bool {
-	return mode == ModeBundle || (mode == ModeConvertFormat && outputFormat == FormatIIFE)
-}
-
 func ShouldCallRuntimeRequire(mode Mode, outputFormat Format) bool {
 	return mode == ModeBundle && outputFormat != FormatCommonJS
 }

internal/js_parser/js_parser.go

@@ -336,6 +336,7 @@ type optionsThatSupportStructuralEquality struct {
 	minifyIdentifiers       bool
 	omitRuntimeForTests     bool
 	ignoreDCEAnnotations    bool
+	treeShaking             bool
 	preserveUnusedImportsTS bool
 	useDefineForClassFields config.MaybeBool
 }
@@ -361,6 +362,7 @@ func OptionsFromConfig(options *config.Options) Options {
 			minifyIdentifiers:       options.MinifyIdentifiers,
 			omitRuntimeForTests:     options.OmitRuntimeForTests,
 			ignoreDCEAnnotations:    options.IgnoreDCEAnnotations,
+			treeShaking:             options.TreeShaking,
 			preserveUnusedImportsTS: options.PreserveUnusedImportsTS,
 			useDefineForClassFields: options.UseDefineForClassFields,
 		},
@@ -13904,11 +13906,11 @@ func Parse(log logger.Log, source logger.Source, options Options) (result js_ast
 	// single pass, but it turns out it's pretty much impossible to do this
 	// correctly while handling arrow functions because of the grammar
 	// ambiguities.
-	if !config.IsTreeShakingEnabled(p.options.mode, p.options.outputFormat) {
-		// When not bundling, everything comes in a single part
+	if !p.options.treeShaking {
+		// When tree shaking is disabled, everything comes in a single part
 		parts = p.appendPart(parts, stmts)
 	} else {
-		// When bundling, each top-level statement is potentially a separate part
+		// When tree shaking is enabled, each top-level statement is potentially a separate part
 		for _, stmt := range stmts {
 			switch s := stmt.Data.(type) {
 			case *js_ast.SLocal:

lib/shared/common.ts

@@ -108,7 +108,8 @@ function pushCommonFlags(flags: string[], options: CommonOptions, keys: OptionKe
   let minifyWhitespace = getFlag(options, keys, 'minifyWhitespace', mustBeBoolean);
   let minifyIdentifiers = getFlag(options, keys, 'minifyIdentifiers', mustBeBoolean);
   let charset = getFlag(options, keys, 'charset', mustBeString);
-  let treeShaking = getFlag(options, keys, 'treeShaking', mustBeStringOrBoolean);
+  let treeShaking = getFlag(options, keys, 'treeShaking', mustBeBoolean);
+  let ignoreAnnotations = getFlag(options, keys, 'ignoreAnnotations', mustBeBoolean);
   let jsx = getFlag(options, keys, 'jsx', mustBeString);
   let jsxFactory = getFlag(options, keys, 'jsxFactory', mustBeString);
   let jsxFragment = getFlag(options, keys, 'jsxFragment', mustBeString);
@@ -131,7 +132,8 @@ function pushCommonFlags(flags: string[], options: CommonOptions, keys: OptionKe
   if (minifyWhitespace) flags.push('--minify-whitespace');
   if (minifyIdentifiers) flags.push('--minify-identifiers');
   if (charset) flags.push(`--charset=${charset}`);
-  if (treeShaking !== void 0 && treeShaking !== true) flags.push(`--tree-shaking=${treeShaking}`);
+  if (treeShaking !== void 0) flags.push(`--tree-shaking=${treeShaking}`);
+  if (ignoreAnnotations) flags.push(`--ignore-annotations`);
 
   if (jsx) flags.push(`--jsx=${jsx}`);
   if (jsxFactory) flags.push(`--jsx-factory=${jsxFactory}`);

lib/shared/types.ts

@@ -3,7 +3,6 @@ export type Format = 'iife' | 'cjs' | 'esm';
 export type Loader = 'js' | 'jsx' | 'ts' | 'tsx' | 'css' | 'json' | 'text' | 'base64' | 'file' | 'dataurl' | 'binary' | 'default';
 export type LogLevel = 'verbose' | 'debug' | 'info' | 'warning' | 'error' | 'silent';
 export type Charset = 'ascii' | 'utf8';
-export type TreeShaking = true | 'ignore-annotations';
 
 interface CommonOptions {
   sourcemap?: boolean | 'inline' | 'external' | 'both';
@@ -20,7 +19,8 @@ interface CommonOptions {
   minifyIdentifiers?: boolean;
   minifySyntax?: boolean;
   charset?: Charset;
-  treeShaking?: TreeShaking;
+  treeShaking?: boolean;
+  ignoreAnnotations?: boolean;
 
   jsx?: 'transform' | 'preserve';
   jsxFactory?: string;

pkg/api/api.go

@@ -235,7 +235,8 @@ type TreeShaking uint8
 
 const (
 	TreeShakingDefault TreeShaking = iota
-	TreeShakingIgnoreAnnotations
+	TreeShakingFalse
+	TreeShakingTrue
 )
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -258,6 +259,7 @@ type BuildOptions struct {
 	MinifySyntax      bool
 	Charset           Charset
 	TreeShaking       TreeShaking
+	IgnoreAnnotations bool
 	LegalComments     LegalComments
 
 	JSXMode     JSXMode
@@ -366,6 +368,7 @@ type TransformOptions struct {
 	MinifySyntax      bool
 	Charset           Charset
 	TreeShaking       TreeShaking
+	IgnoreAnnotations bool
 	LegalComments     LegalComments
 
 	JSXMode     JSXMode

pkg/api/api_impl.go

@@ -204,11 +204,19 @@ func validateASCIIOnly(value Charset) bool {
 	}
 }
 
-func validateIgnoreDCEAnnotations(value TreeShaking) bool {
+func validateTreeShaking(value TreeShaking, bundle bool, format Format) bool {
 	switch value {
 	case TreeShakingDefault:
+		// If we're in an IIFE then there's no way to concatenate additional code
+		// to the end of our output so we assume tree shaking is safe. And when
+		// bundling we assume that tree shaking is safe because if you want to add
+		// code to the bundle, you should be doing that by including it in the
+		// bundle instead of concatenating it afterward, so we also assume tree
+		// shaking is safe then. Otherwise we assume tree shaking is not safe.
+		return bundle || format == FormatIIFE
+	case TreeShakingFalse:
 		return false
-	case TreeShakingIgnoreAnnotations:
+	case TreeShakingTrue:
 		return true
 	default:
 		panic("Invalid tree shaking")
@@ -828,7 +836,8 @@ func rebuildImpl(
 		MinifyIdentifiers:     buildOpts.MinifyIdentifiers,
 		AllowOverwrite:        buildOpts.AllowOverwrite,
 		ASCIIOnly:             validateASCIIOnly(buildOpts.Charset),
-		IgnoreDCEAnnotations:  validateIgnoreDCEAnnotations(buildOpts.TreeShaking),
+		IgnoreDCEAnnotations:  buildOpts.IgnoreAnnotations,
+		TreeShaking:           validateTreeShaking(buildOpts.TreeShaking, buildOpts.Bundle, buildOpts.Format),
 		GlobalName:            validateGlobalName(log, buildOpts.GlobalName),
 		CodeSplitting:         buildOpts.Splitting,
 		OutputFormat:          validateFormat(buildOpts.Format),
@@ -1311,7 +1320,8 @@ func transformImpl(input string, transformOpts TransformOptions) TransformResult
 		RemoveWhitespace:        transformOpts.MinifyWhitespace,
 		MinifyIdentifiers:       transformOpts.MinifyIdentifiers,
 		ASCIIOnly:               validateASCIIOnly(transformOpts.Charset),
-		IgnoreDCEAnnotations:    validateIgnoreDCEAnnotations(transformOpts.TreeShaking),
+		IgnoreDCEAnnotations:    transformOpts.IgnoreAnnotations,
+		TreeShaking:             validateTreeShaking(transformOpts.TreeShaking, false /* bundle */, transformOpts.Format),
 		AbsOutputFile:           transformOpts.Sourcefile + "-out",
 		KeepNames:               transformOpts.KeepNames,
 		UseDefineForClassFields: useDefineForClassFieldsTS,

pkg/cli/cli_impl.go

@@ -144,10 +144,19 @@ func parseOptionsImpl(
 			}
 			name := arg[len("--tree-shaking="):]
 			switch name {
-			case "ignore-annotations":
-				*value = api.TreeShakingIgnoreAnnotations
+			case "false":
+				*value = api.TreeShakingFalse
+			case "true":
+				*value = api.TreeShakingTrue
 			default:
-				return fmt.Errorf("Invalid tree shaking value: %q (valid: ignore-annotations)", name), nil
+				return fmt.Errorf("Invalid tree shaking value: %q (valid: false, true)", name), nil
+			}
+
+		case arg == "--ignore-annotations":
+			if buildOpts != nil {
+				buildOpts.IgnoreAnnotations = true
+			} else {
+				transformOpts.IgnoreAnnotations = true
 			}
 
 		case arg == "--keep-names":

scripts/js-api-tests.js

@@ -1984,7 +1984,7 @@ console.log("success");
       },
       write: false,
       bundle: true,
-      treeShaking: 'ignore-annotations',
+      ignoreAnnotations: true,
     })
     assert.strictEqual(outputFiles[0].text, `(() => {
   // <stdin>
@@ -3205,28 +3205,84 @@ let transformTests = {
     assert.strictEqual(code2, `/* @__PURE__ */ factory(fragment, null, /* @__PURE__ */ factory("div", null));\n`)
   },
 
+  // Note: tree shaking is disabled when the output format isn't IIFE
   async treeShakingDefault({ esbuild }) {
+    const { code } = await esbuild.transform(`var unused = 123`, {
+      loader: 'jsx',
+      format: 'esm',
+      treeShaking: undefined,
+    })
+    assert.strictEqual(code, `var unused = 123;\n`)
+  },
+
+  async treeShakingFalse({ esbuild }) {
+    const { code } = await esbuild.transform(`var unused = 123`, {
+      loader: 'jsx',
+      format: 'esm',
+      treeShaking: false,
+    })
+    assert.strictEqual(code, `var unused = 123;\n`)
+  },
+
+  async treeShakingTrue({ esbuild }) {
+    const { code } = await esbuild.transform(`var unused = 123`, {
+      loader: 'jsx',
+      format: 'esm',
+      treeShaking: true,
+    })
+    assert.strictEqual(code, ``)
+  },
+
+  // Note: tree shaking is enabled when the output format is IIFE
+  async treeShakingDefaultIIFE({ esbuild }) {
+    const { code } = await esbuild.transform(`var unused = 123`, {
+      loader: 'jsx',
+      format: 'iife',
+      treeShaking: undefined,
+    })
+    assert.strictEqual(code, `(() => {\n})();\n`)
+  },
+
+  async treeShakingFalseIIFE({ esbuild }) {
+    const { code } = await esbuild.transform(`var unused = 123`, {
+      loader: 'jsx',
+      format: 'iife',
+      treeShaking: false,
+    })
+    assert.strictEqual(code, `(() => {\n  var unused = 123;\n})();\n`)
+  },
+
+  async treeShakingTrueIIFE({ esbuild }) {
+    const { code } = await esbuild.transform(`var unused = 123`, {
+      loader: 'jsx',
+      format: 'iife',
+      treeShaking: true,
+    })
+    assert.strictEqual(code, `(() => {\n})();\n`)
+  },
+
+  async ignoreAnnotationsDefault({ esbuild }) {
     const { code } = await esbuild.transform(`/* @__PURE__ */ fn(); <div/>`, {
       loader: 'jsx',
       minifySyntax: true,
     })
     assert.strictEqual(code, ``)
   },
 
-  async treeShakingTrue({ esbuild }) {
+  async ignoreAnnotationsFalse({ esbuild }) {
     const { code } = await esbuild.transform(`/* @__PURE__ */ fn(); <div/>`, {
       loader: 'jsx',
       minifySyntax: true,
-      treeShaking: true,
+      ignoreAnnotations: false,
     })
     assert.strictEqual(code, ``)
   },
 
-  async treeShakingIgnoreAnnotations({ esbuild }) {
+  async ignoreAnnotationsTrue({ esbuild }) {
     const { code } = await esbuild.transform(`/* @__PURE__ */ fn(); <div/>`, {
       loader: 'jsx',
       minifySyntax: true,
-      treeShaking: 'ignore-annotations',
+      ignoreAnnotations: true,
     })
     assert.strictEqual(code, `fn(), React.createElement("div", null);\n`)
   },