From 36116c18be4c3b0290b7a949a930ba915a492a2c Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Fri, 25 Sep 2020 20:22:42 -0700 Subject: [PATCH] fix #410: silence require.resolve() of externals --- CHANGELOG.md | 13 +++ internal/bundler/bundler.go | 23 +++-- internal/bundler/bundler_default_test.go | 83 +++++++++++++++---- internal/bundler/bundler_ts_test.go | 3 +- .../bundler/snapshots/snapshots_splitting.txt | 23 +++++ internal/js_ast/js_ast.go | 8 ++ internal/js_parser/js_parser.go | 45 +++++++++- internal/js_printer/js_printer.go | 13 +++ 8 files changed, 187 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cec26d82fec..c5952090fe8 100644 --- a/CHANGELOG.md +++ b/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)) diff --git a/internal/bundler/bundler.go b/internal/bundler/bundler.go index ae038c038c2..2b3bd603058 100644 --- a/internal/bundler/bundler.go +++ b/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)" diff --git a/internal/bundler/bundler_default_test.go b/internal/bundler/bundler_default_test.go index 2d1b704f690..2f5f08f196b 100644 --- a/internal/bundler/bundler_default_test.go +++ b/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" +`, + }) +} diff --git a/internal/bundler/bundler_ts_test.go b/internal/bundler/bundler_ts_test.go index a86804877bd..604a0f3b4a1 100644 --- a/internal/bundler/bundler_ts_test.go +++ b/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" +`, }) } diff --git a/internal/bundler/snapshots/snapshots_splitting.txt b/internal/bundler/snapshots/snapshots_splitting.txt index a5b362c86b0..e847d2d3cea 100644 --- a/internal/bundler/snapshots/snapshots_splitting.txt +++ b/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 { diff --git a/internal/js_ast/js_ast.go b/internal/js_ast/js_ast.go index 2ec84a45487..7cac8b33a69 100644 --- a/internal/js_ast/js_ast.go +++ b/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 { diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go index 8dde9c810aa..d440e88f3f4 100644 --- a/internal/js_parser/js_parser.go +++ b/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)") } } diff --git a/internal/js_printer/js_printer.go b/internal/js_printer/js_printer.go index be781e8c1e8..26c5e869b75 100644 --- a/internal/js_printer/js_printer.go +++ b/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 {