fix #893: add the "import().catch()" pattern

CHANGELOG.md

@@ -27,6 +27,10 @@
 
     This case previously wasn't handled because doing this is a compile error in TypeScript code. However, JavaScript does allow this so esbuild needs to be able to handle this. This edge case should now work correctly with this release.
 
+* Do not warn about dynamic imports when `.catch()` is detected ([#893](https://github.com/evanw/esbuild/issues/893))
+
+    Previously esbuild avoids warning about unbundled `import()` expressions when using the `try { await import(_) }` pattern, since presumably the `try` block is there to handle the run-time failure of the `import()` expression failing. This release adds some new patterns that will also suppress the warning: `import(_).catch(_)`, `import(_).then(_).catch(_)`, and `import(_).then(_, _)`.
+
 * CSS namespaces are no longer supported
 
     [CSS namespaces](https://developer.mozilla.org/en-US/docs/Web/CSS/@namespace) are a weird feature that appears to only really be useful for styling XML. And the world has moved on from XHTML to HTML5 so pretty much no one uses CSS namespaces anymore. They are also complicated to support in a bundler because CSS namespaces are file-scoped, which means:

internal/bundler/bundler_default_test.go

@@ -847,12 +847,12 @@ func TestRequireAndDynamicImportInvalidTemplate(t *testing.T) {
 		},
 		expectedScanLog: `entry.js: warning: This call to "require" will not be bundled because the argument is not a string literal (surround with a try/catch to silence this warning)
 entry.js: warning: This call to "require" will not be bundled because the argument is not a string literal (surround with a try/catch to silence this warning)
-entry.js: warning: This dynamic import will not be bundled because the argument is not a string literal
-entry.js: warning: This dynamic import will not be bundled because the argument is not a string literal
+entry.js: warning: This dynamic import will not be bundled because the argument is not a string literal (use "import().catch()" to silence this warning)
+entry.js: warning: This dynamic import will not be bundled because the argument is not a string literal (use "import().catch()" to silence this warning)
 entry.js: warning: This dynamic import will not be bundled because the argument is not a string literal (surround with a try/catch to silence this warning)
 entry.js: warning: This dynamic import will not be bundled because the argument is not a string literal (surround with a try/catch to silence this warning)
-entry.js: warning: This dynamic import will not be bundled because the argument is not a string literal
-entry.js: warning: This dynamic import will not be bundled because the argument is not a string literal
+entry.js: warning: This dynamic import will not be bundled because the argument is not a string literal (use "import().catch()" to silence this warning)
+entry.js: warning: This dynamic import will not be bundled because the argument is not a string literal (use "import().catch()" to silence this warning)
 `,
 	})
 }
@@ -983,25 +983,27 @@ func TestConditionalImport(t *testing.T) {
 	default_suite.expectBundled(t, bundled{
 		files: map[string]string{
 			"/a.js": `
-				import(x ? 'a' : y ? './b' : 'c')
-				import(x ? y ? 'a' : './b' : c)
+				import(x ? 'a' : y ? './import' : 'c')
 			`,
 			"/b.js": `
+				import(x ? y ? 'a' : './import' : c)
+			`,
+			"/import.js": `
 				exports.foo = 213
 			`,
 		},
-		entryPaths: []string{"/a.js"},
+		entryPaths: []string{"/a.js", "/b.js"},
 		options: config.Options{
-			Mode:          config.ModeBundle,
-			AbsOutputFile: "/out.js",
+			Mode:         config.ModeBundle,
+			AbsOutputDir: "/out",
 			ExternalModules: config.ExternalModules{
 				NodeModules: map[string]bool{
 					"a": true,
 					"c": true,
 				},
 			},
 		},
-		expectedScanLog: `a.js: warning: This dynamic import will not be bundled because the argument is not a string literal
+		expectedScanLog: `b.js: warning: This dynamic import will not be bundled because the argument is not a string literal (use "import().catch()" to silence this warning)
 `,
 	})
 }
@@ -1270,11 +1272,29 @@ func TestImportInsideTry(t *testing.T) {
 			Mode:          config.ModeBundle,
 			AbsOutputFile: "/out.js",
 		},
-		expectedScanLog: `entry.js: warning: This dynamic import will not be bundled because the argument is not a string literal
+		expectedScanLog: `entry.js: warning: This dynamic import will not be bundled because the argument is not a string literal (use "import().catch()" to silence this warning)
 `,
 	})
 }
 
+// Test a workaround for code using "import().catch()"
+func TestImportThenCatch(t *testing.T) {
+	default_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/entry.js": `
+				import(name).then(pass, fail)
+				import(name).then(pass).catch(fail)
+				import(name).catch(fail)
+			`,
+		},
+		entryPaths: []string{"/entry.js"},
+		options: config.Options{
+			Mode:          config.ModeBundle,
+			AbsOutputFile: "/out.js",
+		},
+	})
+}
+
 func TestSourceMap(t *testing.T) {
 	default_suite.expectBundled(t, bundled{
 		files: map[string]string{

internal/bundler/snapshots/snapshots_default.txt

@@ -251,15 +251,23 @@ var {bar} = require_bar();
 
 ================================================================================
 TestConditionalImport
----------- /out.js ----------
-// b.js
-var require_b = __commonJS((exports) => {
+---------- /out/a.js ----------
+// import.js
+var require_import = __commonJS((exports) => {
   exports.foo = 213;
 });
 
 // a.js
-x ? import("a") : y ? Promise.resolve().then(() => __toModule(require_b())) : import("c");
-x ? y ? import("a") : Promise.resolve().then(() => __toModule(require_b())) : import(c);
+x ? import("a") : y ? Promise.resolve().then(() => __toModule(require_import())) : import("c");
+
+---------- /out/b.js ----------
+// import.js
+var require_import = __commonJS((exports) => {
+  exports.foo = 213;
+});
+
+// b.js
+x ? y ? import("a") : Promise.resolve().then(() => __toModule(require_import())) : import(c);
 
 ================================================================================
 TestConditionalRequire
@@ -1029,6 +1037,14 @@ var Internal = () => /* @__PURE__ */ h(p, null, " Test 2 ");
 var App = () => /* @__PURE__ */ h(p, null, " ", /* @__PURE__ */ h(Internal, null), " T ");
 render(/* @__PURE__ */ h(App, null), document.getElementById("app"));
 
+================================================================================
+TestImportThenCatch
+---------- /out.js ----------
+// entry.js
+import(name).then(pass, fail);
+import(name).then(pass).catch(fail);
+import(name).catch(fail);
+
 ================================================================================
 TestImportWithHashInPath
 ---------- /out/entry.js ----------

internal/js_parser/js_parser.go

@@ -207,6 +207,10 @@ type parser struct {
 	// warnings about non-string import paths will be omitted inside try blocks.
 	awaitTarget js_ast.E
 
+	// This helps recognize the "import().catch()" pattern. We also try to avoid
+	// warning about this just like the "try { await import() }" pattern.
+	thenCatchChain thenCatchChain
+
 	// This helps recognize the "require.someProperty" pattern. If this pattern is
 	// present and the output format is CommonJS, we avoid generating a warning
 	// about an unbundled use of "require".
@@ -246,6 +250,12 @@ type parser struct {
 	afterArrowBodyLoc logger.Loc
 }
 
+type thenCatchChain struct {
+	nextTarget      js_ast.E
+	hasMultipleArgs bool
+	hasCatch        bool
+}
+
 // This is used as part of an incremental build cache key. Some of these values
 // can potentially change between builds if they are derived from nearby
 // "package.json" or "tsconfig.json" files that were changed since the last
@@ -10644,6 +10654,7 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
 
 	case *js_ast.EDot:
 		isDeleteTarget := e == p.deleteTarget
+		isCallTarget := e == p.callTarget
 
 		// Check both user-specified defines and known globals
 		if defines, ok := p.options.defines.DotDefines[e.Name]; ok {
@@ -10676,7 +10687,21 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
 			p.cjsDotTarget = e.Target.Data
 		}
 
-		isCallTarget := e == p.callTarget
+		// Track ".then().catch()" chains
+		if isCallTarget && p.thenCatchChain.nextTarget == e {
+			if e.Name == "catch" {
+				p.thenCatchChain = thenCatchChain{
+					nextTarget: e.Target.Data,
+					hasCatch:   true,
+				}
+			} else if e.Name == "then" {
+				p.thenCatchChain = thenCatchChain{
+					nextTarget: e.Target.Data,
+					hasCatch:   p.thenCatchChain.hasCatch || p.thenCatchChain.hasMultipleArgs,
+				}
+			}
+		}
+
 		target, out := p.visitExprInOut(e.Target, exprIn{
 			hasChainParent: e.OptionalChain == js_ast.OptionalChainContinue,
 		})
@@ -10973,6 +10998,7 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
 
 	case *js_ast.EImport:
 		isAwaitTarget := e == p.awaitTarget
+		isThenCatchTarget := e == p.thenCatchChain.nextTarget && p.thenCatchChain.hasCatch
 		e.Expr = p.visitExpr(e.Expr)
 
 		return p.maybeTransposeIfExprChain(e.Expr, func(arg js_ast.Expr) js_ast.Expr {
@@ -10995,15 +11021,19 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
 			}
 
 			if p.options.mode == config.ModeBundle {
-				// Heuristic: omit warnings inside try/catch blocks because presumably
-				// the try/catch statement is there to handle the potential run-time
-				// error from the unbundled "await import()" call failing.
-				omitWarnings := p.fnOrArrowDataVisit.tryBodyCount != 0 && isAwaitTarget
+				// Heuristic: omit warnings when using "await import()" inside a try
+				// block because presumably the try block is there to handle the
+				// potential run-time error from the unbundled "await import()" call
+				// failing. Also support the "import().then(pass, fail)" pattern as
+				// well as the "import().catch(fail)" pattern.
+				omitWarnings := (p.fnOrArrowDataVisit.tryBodyCount != 0 && isAwaitTarget) || isThenCatchTarget
 
 				if !omitWarnings {
 					text := "This dynamic import will not be bundled because the argument is not a string literal"
 					if isAwaitTarget {
 						text += " (surround with a try/catch to silence this warning)"
+					} else {
+						text += " (use \"import().catch()\" to silence this warning)"
 					}
 					r := js_lexer.RangeOfIdentifier(p.source, expr.Loc)
 					p.log.AddRangeWarning(&p.source, r, text)
@@ -11062,6 +11092,13 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
 	case *js_ast.ECall:
 		p.callTarget = e.Target.Data
 
+		// Track ".then().catch()" chains
+		p.thenCatchChain = thenCatchChain{
+			nextTarget:      e.Target.Data,
+			hasMultipleArgs: len(e.Args) >= 2,
+			hasCatch:        p.thenCatchChain.nextTarget == e && p.thenCatchChain.hasCatch,
+		}
+
 		// Prepare to recognize "require.resolve()" calls
 		couldBeRequireResolve := false
 		if len(e.Args) == 1 && p.options.mode != config.ModePassThrough {