From 8c83fdf41dfb71f0c52bdd328e371e83644a69a9 Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Thu, 2 Jun 2022 11:05:02 -0400 Subject: [PATCH] fix #2231: log about indirect `require` usage --- CHANGELOG.md | 19 ++++++++++++ internal/bundler/bundler_default_test.go | 30 +++++++++++++++++++ .../bundler/snapshots/snapshots_loader.txt | 18 +++++++++++ internal/js_parser/js_parser.go | 20 +++++++++---- internal/logger/msg_ids.go | 5 ++++ 5 files changed, 86 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 46ad085c525..37a86b1ec9a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,25 @@ // New output (with --minify) ``` +* Add log messages for indirect `require` references ([#2231](https://github.com/evanw/esbuild/issues/2231)) + + A long time ago esbuild used to warn about indirect uses of `require` because they break esbuild's ability to analyze the dependencies of the code and cause dependencies to not be bundled, resulting in a potentially broken bundle. However, this warning was removed because many people wanted the warning to be removed. Some packages have code that uses `require` like this but on a code path that isn't used at run-time, so their code still happens to work even though the bundle is incomplete. For example, the following code will _not_ bundle `bindings`: + + ```js + // Prevent React Native packager from seeing modules required with this + const nodeRequire = require; + + function getRealmConstructor(environment) { + switch (environment) { + case "node.js": + case "electron": + return nodeRequire("bindings")("realm.node").Realm; + } + } + ``` + + Version 0.11.11 of esbuild removed this warning, which means people no longer have a way to know at compile time whether their bundle is broken in this way. Now that esbuild has custom log message levels, this warning can be added back in a way that should make both people happy. With this release, there is now a log message for this that defaults to the `debug` log level, which normally isn't visible. You can either do `--log-override:indirect-require=warning` to make this log message a warning (and therefore visible) or use `--log-level=debug` to see this and all other `debug` log messages. + ## 0.14.42 * Fix a parser hang on invalid CSS ([#2276](https://github.com/evanw/esbuild/issues/2276)) diff --git a/internal/bundler/bundler_default_test.go b/internal/bundler/bundler_default_test.go index 2a6ebe26bba..64e1d62790f 100644 --- a/internal/bundler/bundler_default_test.go +++ b/internal/bundler/bundler_default_test.go @@ -6288,3 +6288,33 @@ func TestMangleQuotedPropsMinifySyntax(t *testing.T) { }, }) } + +func TestIndirectRequireMessage(t *testing.T) { + loader_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/array.js": `let x = [require]`, + "/assign.js": `require = x`, + "/ident.js": `let x = require`, + + // These shouldn't log anything: https://github.com/evanw/esbuild/issues/812 + "/dot.js": `let x = require.cache`, + "/index.js": `let x = require[cache]`, + }, + entryPaths: []string{ + "/array.js", + "/assign.js", + "/dot.js", + "/ident.js", + "/index.js", + }, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputDir: "/out", + }, + debugLogs: true, + expectedScanLog: `array.js: DEBUG: Indirect calls to "require" will not be bundled +assign.js: DEBUG: Indirect calls to "require" will not be bundled +ident.js: DEBUG: Indirect calls to "require" will not be bundled +`, + }) +} diff --git a/internal/bundler/snapshots/snapshots_loader.txt b/internal/bundler/snapshots/snapshots_loader.txt index cc9c761d24f..0ad229920d5 100644 --- a/internal/bundler/snapshots/snapshots_loader.txt +++ b/internal/bundler/snapshots/snapshots_loader.txt @@ -40,6 +40,24 @@ a:after { content: "entry2"; } +================================================================================ +TestIndirectRequireMessage +---------- /out/array.js ---------- + +---------- /out/assign.js ---------- +// assign.js +__require = x; + +---------- /out/dot.js ---------- +// dot.js +var x = __require.cache; + +---------- /out/ident.js ---------- + +---------- /out/index.js ---------- +// index.js +var x = __require[cache]; + ================================================================================ TestJSXPreserveCapitalLetter ---------- /out.js ---------- diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go index baa0ff5993d..304fcea2146 100644 --- a/internal/js_parser/js_parser.go +++ b/internal/js_parser/js_parser.go @@ -143,12 +143,13 @@ type parser struct { // The visit pass binds identifiers to declared symbols, does constant // folding, substitutes compile-time variable definitions, and lowers certain // syntactic constructs as appropriate. - stmtExprValue js_ast.E - callTarget js_ast.E - templateTag js_ast.E - deleteTarget js_ast.E - loopBody js_ast.S - moduleScope *js_ast.Scope + stmtExprValue js_ast.E + callTarget js_ast.E + dotOrIndexTarget js_ast.E + templateTag js_ast.E + deleteTarget js_ast.E + loopBody js_ast.S + moduleScope *js_ast.Scope // This helps recognize the "await import()" pattern. When this is present, // warnings about non-string import paths will be omitted inside try blocks. @@ -12619,6 +12620,7 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO } } + p.dotOrIndexTarget = e.Target.Data target, out := p.visitExprInOut(e.Target, exprIn{ hasChainParent: e.OptionalChain == js_ast.OptionalChainContinue, }) @@ -12703,6 +12705,7 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO } } + p.dotOrIndexTarget = e.Target.Data target, out := p.visitExprInOut(e.Target, exprIn{ hasChainParent: e.OptionalChain == js_ast.OptionalChainContinue, }) @@ -14234,6 +14237,11 @@ func (p *parser) handleIdentifier(loc logger.Loc, e *js_ast.EIdentifier, opts id // Swap references to the global "require" function with our "__require" stub if ref == p.requireRef && !opts.isCallTarget { + if p.options.mode == config.ModeBundle && p.source.Index != runtime.SourceIndex && e != p.dotOrIndexTarget { + p.log.AddID(logger.MsgID_JS_IndirectRequire, logger.Debug, &p.tracker, js_lexer.RangeOfIdentifier(p.source, loc), + "Indirect calls to \"require\" will not be bundled") + } + return p.valueToSubstituteForRequire(loc) } diff --git a/internal/logger/msg_ids.go b/internal/logger/msg_ids.go index d98d091a531..8c8440453c1 100644 --- a/internal/logger/msg_ids.go +++ b/internal/logger/msg_ids.go @@ -25,6 +25,7 @@ const ( MsgID_JS_EqualsNewObject MsgID_JS_HTMLCommentInJS MsgID_JS_ImpossibleTypeof + MsgID_JS_IndirectRequire MsgID_JS_PrivateNameWillThrow MsgID_JS_SemicolonAfterReturn MsgID_JS_SuspiciousBooleanNot @@ -110,6 +111,8 @@ func StringToMsgIDs(str string, logLevel LogLevel, overrides map[MsgID]LogLevel) overrides[MsgID_JS_HTMLCommentInJS] = logLevel case "impossible-typeof": overrides[MsgID_JS_ImpossibleTypeof] = logLevel + case "indirect-require": + overrides[MsgID_JS_IndirectRequire] = logLevel case "private-name-will-throw": overrides[MsgID_JS_PrivateNameWillThrow] = logLevel case "semicolon-after-return": @@ -216,6 +219,8 @@ func MsgIDToString(id MsgID) string { return "html-comment-in-js" case MsgID_JS_ImpossibleTypeof: return "impossible-typeof" + case MsgID_JS_IndirectRequire: + return "indirect-require" case MsgID_JS_PrivateNameWillThrow: return "private-name-will-throw" case MsgID_JS_SemicolonAfterReturn: