fix #410: silence require.resolve() of externals

CHANGELOG.md

@@ -6,6 +6,19 @@
 
     This release fixes an issue where a single build operation containing multiple entry points and a shared JSON file which is used by more than one of those entry points can generate incorrect code for the JSON file when code splitting is disabled. The problem was not cloning the AST representing the JSON file before mutating it.
 
+* Silence warnings about `require.resolve()` for external paths ([#410](https://github.com/evanw/esbuild/issues/410))
+
+    Bundling code containing a call to node's [`require.resolve()`](https://nodejs.org/api/modules.html#modules_require_resolve_request_options) function causes a warning because it's an unsupported use of `require` that does not end up being bundled. For example, the following code will likely have unexpected behavior if `foo` ends up being bundled because the `require()` call is evaluated at bundle time but the `require.resolve()` call is evaluated at run time:
+
+    ```js
+    let foo = {
+      path: require.resolve('foo'),
+      module: require('foo'),
+    };
+    ```
+
+    These warnings can already be disabled by surrounding the code with a `try`/`catch` statement. With this release, these warnings can now also be disabled by marking the path as external.
+
 ## 0.7.5
 
 * Fix an issue with automatic semicolon insertion after `let` ([#409](https://github.com/evanw/esbuild/issues/409))

internal/bundler/bundler.go

@@ -260,8 +260,7 @@ func parseFile(args parseArgs) {
 		result.resolveResults = make([]*resolver.ResolveResult, len(result.file.ast.ImportRecords))
 
 		if len(result.file.ast.ImportRecords) > 0 {
-			cacheRequire := make(map[string]*resolver.ResolveResult)
-			cacheImport := make(map[string]*resolver.ResolveResult)
+			resolverCache := make(map[js_ast.ImportKind]map[string]*resolver.ResolveResult)
 
 			// Resolve relative to the parent directory of the source file with the
 			// import path. Just use the current directory if the source file is virtual.
@@ -288,9 +287,10 @@ func parseFile(args parseArgs) {
 				}
 
 				// Cache the path in case it's imported multiple times in this file
-				cache := cacheImport
-				if record.Kind == js_ast.ImportRequire {
-					cache = cacheRequire
+				cache, ok := resolverCache[record.Kind]
+				if !ok {
+					cache = make(map[string]*resolver.ResolveResult)
+					resolverCache[record.Kind] = cache
 				}
 				if resolveResult, ok := cache[record.Path.Text]; ok {
 					result.resolveResults[importRecordIndex] = resolveResult
@@ -301,6 +301,16 @@ func parseFile(args parseArgs) {
 				resolveResult := args.res.Resolve(sourceDir, record.Path.Text, record.Kind)
 				cache[record.Path.Text] = resolveResult
 
+				// All "require.resolve()" imports should be external because we don't
+				// want to waste effort traversing into them
+				if record.Kind == js_ast.ImportRequireResolve {
+					if !record.IsInsideTryBody && (resolveResult == nil || !resolveResult.IsExternal) {
+						args.log.AddRangeWarning(&source, source.RangeOfString(record.Loc),
+							fmt.Sprintf("%q should be marked as external for use with \"require.resolve\"", record.Path.Text))
+					}
+					continue
+				}
+
 				if resolveResult == nil {
 					// Failed imports inside a try/catch are silently turned into
 					// external imports instead of causing errors. This matches a common
@@ -309,6 +319,9 @@ func parseFile(args parseArgs) {
 					if !record.IsInsideTryBody {
 						r := source.RangeOfString(record.Loc)
 						hint := ""
+						if resolver.IsPackagePath(record.Path.Text) {
+							hint = " (mark it as external to exclude it from the bundle)"
+						}
 						if args.options.Platform != config.PlatformNode {
 							if _, ok := resolver.BuiltInNodeModules[record.Path.Text]; ok {
 								hint = " (set platform to \"node\" when building for node)"

internal/bundler/bundler_default_test.go

@@ -906,7 +906,7 @@ func TestRequireWithoutCall(t *testing.T) {
 			Mode:          config.ModeBundle,
 			AbsOutputFile: "/out.js",
 		},
-		expectedScanLog: `/entry.js: warning: Indirect calls to "require" will not be bundled
+		expectedScanLog: `/entry.js: warning: Indirect calls to "require" will not be bundled (surround with a try/catch to silence this warning)
 `,
 	})
 }
@@ -926,7 +926,7 @@ func TestNestedRequireWithoutCall(t *testing.T) {
 			Mode:          config.ModeBundle,
 			AbsOutputFile: "/out.js",
 		},
-		expectedScanLog: `/entry.js: warning: Indirect calls to "require" will not be bundled
+		expectedScanLog: `/entry.js: warning: Indirect calls to "require" will not be bundled (surround with a try/catch to silence this warning)
 `,
 	})
 }
@@ -1121,14 +1121,14 @@ func TestTypeofRequireBadPatterns(t *testing.T) {
 			Mode:          config.ModeBundle,
 			AbsOutputFile: "/out.js",
 		},
-		expectedScanLog: `/entry.js: warning: Indirect calls to "require" will not be bundled
-/entry.js: warning: Indirect calls to "require" will not be bundled
-/entry.js: warning: Indirect calls to "require" will not be bundled
-/entry.js: warning: Indirect calls to "require" will not be bundled
-/entry.js: warning: Indirect calls to "require" will not be bundled
-/entry.js: warning: Indirect calls to "require" will not be bundled
-/entry.js: warning: Indirect calls to "require" will not be bundled
-/entry.js: warning: Indirect calls to "require" will not be bundled
+		expectedScanLog: `/entry.js: warning: Indirect calls to "require" will not be bundled (surround with a try/catch to silence this warning)
+/entry.js: warning: Indirect calls to "require" will not be bundled (surround with a try/catch to silence this warning)
+/entry.js: warning: Indirect calls to "require" will not be bundled (surround with a try/catch to silence this warning)
+/entry.js: warning: Indirect calls to "require" will not be bundled (surround with a try/catch to silence this warning)
+/entry.js: warning: Indirect calls to "require" will not be bundled (surround with a try/catch to silence this warning)
+/entry.js: warning: Indirect calls to "require" will not be bundled (surround with a try/catch to silence this warning)
+/entry.js: warning: Indirect calls to "require" will not be bundled (surround with a try/catch to silence this warning)
+/entry.js: warning: Indirect calls to "require" will not be bundled (surround with a try/catch to silence this warning)
 `,
 	})
 }
@@ -1883,12 +1883,12 @@ func TestExternalModuleExclusionScopedPackage(t *testing.T) {
 				},
 			},
 		},
-		expectedScanLog: `/index.js: error: Could not resolve "@a1-a2"
-/index.js: error: Could not resolve "@b1"
-/index.js: error: Could not resolve "@b1/b2-b3"
-/index.js: error: Could not resolve "@c1"
-/index.js: error: Could not resolve "@c1/c2"
-/index.js: error: Could not resolve "@c1/c2/c3-c4"
+		expectedScanLog: `/index.js: error: Could not resolve "@a1-a2" (mark it as external to exclude it from the bundle)
+/index.js: error: Could not resolve "@b1" (mark it as external to exclude it from the bundle)
+/index.js: error: Could not resolve "@b1/b2-b3" (mark it as external to exclude it from the bundle)
+/index.js: error: Could not resolve "@c1" (mark it as external to exclude it from the bundle)
+/index.js: error: Could not resolve "@c1/c2" (mark it as external to exclude it from the bundle)
+/index.js: error: Could not resolve "@c1/c2/c3-c4" (mark it as external to exclude it from the bundle)
 `,
 	})
 }
@@ -2811,3 +2811,54 @@ func TestWarningsInsideNodeModules(t *testing.T) {
 `,
 	})
 }
+
+func TestRequireResolve(t *testing.T) {
+	splitting_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/entry.js": `
+				console.log(require.resolve)
+				console.log(require.resolve())
+				console.log(require.resolve(foo))
+				console.log(require.resolve('a', 'b'))
+				console.log(require.resolve('./present-file'))
+				console.log(require.resolve('./missing-file'))
+				console.log(require.resolve('./external-file'))
+				console.log(require.resolve('missing-pkg'))
+				console.log(require.resolve('external-pkg'))
+				console.log(require.resolve('@scope/missing-pkg'))
+				console.log(require.resolve('@scope/external-pkg'))
+				try {
+					console.log(require.resolve('inside-try'))
+				} catch (e) {
+				}
+				if (false) {
+					console.log(require.resolve('dead-code'))
+				}
+			`,
+			"/present-file.js": ``,
+		},
+		entryPaths: []string{"/entry.js"},
+		options: config.Options{
+			Mode:          config.ModeBundle,
+			AbsOutputFile: "/out.js",
+			ExternalModules: config.ExternalModules{
+				AbsPaths: map[string]bool{
+					"/external-file": true,
+				},
+				NodeModules: map[string]bool{
+					"external-pkg":        true,
+					"@scope/external-pkg": true,
+				},
+			},
+		},
+		expectedScanLog: `/entry.js: warning: Indirect calls to "require" will not be bundled (surround with a try/catch to silence this warning)
+/entry.js: warning: Indirect calls to "require" will not be bundled (surround with a try/catch to silence this warning)
+/entry.js: warning: Indirect calls to "require" will not be bundled (surround with a try/catch to silence this warning)
+/entry.js: warning: Indirect calls to "require" will not be bundled (surround with a try/catch to silence this warning)
+/entry.js: warning: "./present-file" should be marked as external for use with "require.resolve"
+/entry.js: warning: "./missing-file" should be marked as external for use with "require.resolve"
+/entry.js: warning: "missing-pkg" should be marked as external for use with "require.resolve"
+/entry.js: warning: "@scope/missing-pkg" should be marked as external for use with "require.resolve"
+`,
+	})
+}

internal/bundler/bundler_ts_test.go

@@ -266,7 +266,8 @@ func TestTSImportMissingFile(t *testing.T) {
 			Mode:          config.ModeBundle,
 			AbsOutputFile: "/out.js",
 		},
-		expectedScanLog: "/entry.ts: error: Could not resolve \"./doesNotExist.ts\"\n",
+		expectedScanLog: `/entry.ts: error: Could not resolve "./doesNotExist.ts"
+`,
 	})
 }
 

internal/bundler/snapshots/snapshots_splitting.txt

@@ -1,3 +1,26 @@
+TestRequireResolve
+---------- /out.js ----------
+// /entry.js
+console.log(require.resolve);
+console.log(require.resolve());
+console.log(require.resolve(foo));
+console.log(require.resolve("a", "b"));
+console.log(require.resolve("./present-file"));
+console.log(require.resolve("./missing-file"));
+console.log(require.resolve("./external-file"));
+console.log(require.resolve("missing-pkg"));
+console.log(require.resolve("external-pkg"));
+console.log(require.resolve("@scope/missing-pkg"));
+console.log(require.resolve("@scope/external-pkg"));
+try {
+  console.log(require.resolve("inside-try"));
+} catch (e) {
+}
+if (false) {
+  console.log(null);
+}
+
+================================================================================
 TestSplittingAssignToLocal
 ---------- /out/a.js ----------
 import {

internal/js_ast/js_ast.go

@@ -593,6 +593,10 @@ type ERequire struct {
 	ImportRecordIndex uint32
 }
 
+type ERequireResolve struct {
+	ImportRecordIndex uint32
+}
+
 type EImport struct {
 	Expr              Expr
 	ImportRecordIndex *uint32
@@ -640,6 +644,7 @@ func (*EAwait) isExpr()             {}
 func (*EYield) isExpr()             {}
 func (*EIf) isExpr()                {}
 func (*ERequire) isExpr()           {}
+func (*ERequireResolve) isExpr()    {}
 func (*EImport) isExpr()            {}
 
 func Assign(a Expr, b Expr) Expr {
@@ -1327,6 +1332,9 @@ const (
 
 	// An "import()" expression with a string argument
 	ImportDynamic
+
+	// A call to "require.resolve()"
+	ImportRequireResolve
 )
 
 type ImportRecord struct {

internal/js_parser/js_parser.go

@@ -175,6 +175,10 @@ type parser struct {
 	typeofRequireEqualsFn       js_ast.E
 	typeofRequireEqualsFnTarget js_ast.E
 
+	// This helps recognize calls to "require.resolve()" which may become
+	// ERequireResolve expressions.
+	resolveCallTarget js_ast.E
+
 	// Temporary variables used for lowering
 	tempRefsToDeclare []tempRef
 	tempRefCount      int
@@ -8957,6 +8961,18 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
 				return thisArgFunc()
 			}
 		}
+
+		// Prepare to recognize "require.resolve()" calls
+		couldBeRequireResolve := false
+		if len(e.Args) == 1 && p.Mode != config.ModePassThrough {
+			if dot, ok := e.Target.Data.(*js_ast.EDot); ok && dot.OptionalChain == js_ast.OptionalChainNone && dot.Name == "resolve" {
+				if _, ok := e.Args[0].Data.(*js_ast.EString); ok {
+					p.resolveCallTarget = dot.Target.Data
+					couldBeRequireResolve = true
+				}
+			}
+		}
+
 		_, wasIdentifierBeforeVisit := e.Target.Data.(*js_ast.EIdentifier)
 		target, out := p.visitExprInOut(e.Target, exprIn{
 			hasChainParent:                     e.OptionalChain == js_ast.OptionalChainContinue,
@@ -8972,6 +8988,30 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO
 			e.Args[i] = arg
 		}
 
+		// Recognize "require.resolve()" calls
+		if couldBeRequireResolve {
+			if dot, ok := e.Target.Data.(*js_ast.EDot); ok {
+				if id, ok := dot.Target.Data.(*js_ast.EIdentifier); ok && id.Ref == p.requireRef {
+					if str, ok := e.Args[0].Data.(*js_ast.EString); ok {
+						// Ignore calls to require.resolve() if the control flow is provably
+						// dead here. We don't want to spend time scanning the required files
+						// if they will never be used.
+						if p.isControlFlowDead {
+							return js_ast.Expr{Loc: expr.Loc, Data: &js_ast.ENull{}}, exprOut{}
+						}
+
+						importRecordIndex := p.addImportRecord(js_ast.ImportRequireResolve, e.Args[0].Loc, js_lexer.UTF16ToString(str.Value))
+						p.importRecords[importRecordIndex].IsInsideTryBody = p.fnOrArrowDataVisit.tryBodyCount != 0
+						p.importRecordsForCurrentPart = append(p.importRecordsForCurrentPart, importRecordIndex)
+
+						// Create a new expression to represent the operation
+						p.ignoreUsage(p.requireRef)
+						return js_ast.Expr{Loc: expr.Loc, Data: &js_ast.ERequireResolve{ImportRecordIndex: importRecordIndex}}, exprOut{}
+					}
+				}
+			}
+		}
+
 		// "foo(1, ...[2, 3], 4)" => "foo(1, 2, 3, 4)"
 		if p.MangleSyntax && hasSpread && in.assignTarget == js_ast.AssignTargetNone {
 			e.Args = inlineSpreadsOfArrayLiterals(e.Args)
@@ -9236,9 +9276,10 @@ func (p *parser) handleIdentifier(loc logger.Loc, assignTarget js_ast.AssignTarg
 			if p.Platform == config.PlatformBrowser {
 				return js_ast.Expr{Loc: loc, Data: &js_ast.EBoolean{Value: false}}
 			}
-		} else {
+		} else if e != p.resolveCallTarget {
 			r := js_lexer.RangeOfIdentifier(p.source, loc)
-			p.log.AddRangeWarning(&p.source, r, "Indirect calls to \"require\" will not be bundled")
+			p.log.AddRangeWarning(&p.source, r,
+				"Indirect calls to \"require\" will not be bundled (surround with a try/catch to silence this warning)")
 		}
 	}
 

internal/js_printer/js_printer.go

@@ -1373,6 +1373,19 @@ func (p *printer) printExpr(expr js_ast.Expr, level js_ast.L, flags int) {
 			p.print(")")
 		}
 
+	case *js_ast.ERequireResolve:
+		wrap := level >= js_ast.LNew || (flags&forbidCall) != 0
+		if wrap {
+			p.print("(")
+		}
+		p.printSpaceBeforeIdentifier()
+		p.print("require.resolve(")
+		p.printQuoted(p.importRecords[e.ImportRecordIndex].Path.Text)
+		p.print(")")
+		if wrap {
+			p.print(")")
+		}
+
 	case *js_ast.EImport:
 		wrap := level >= js_ast.LNew || (flags&forbidCall) != 0
 		if wrap {