From 8fe9e1c7f4b66751b9c9aa5d23e42a1ba57369e7 Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Wed, 14 Apr 2021 17:58:30 -0700 Subject: [PATCH 1/2] remove "require" and "import" warnings --- CHANGELOG.md | 38 ++++ internal/ast/ast.go | 14 +- internal/bundler/bundler.go | 11 +- internal/bundler/bundler_default_test.go | 152 +--------------- internal/bundler/bundler_packagejson_test.go | 8 +- .../bundler/snapshots/snapshots_default.txt | 73 -------- internal/js_parser/js_parser.go | 165 +----------------- 7 files changed, 75 insertions(+), 386 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a42b5f09164..3390ca59369 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,43 @@ # Changelog +## Unreleased + +* Remove warnings about non-bundled use of `require` and `import` ([#1153](https://github.com/evanw/esbuild/issues/1153), [#1142](https://github.com/evanw/esbuild/issues/1142), [#1132](https://github.com/evanw/esbuild/issues/1132), [#1045](https://github.com/evanw/esbuild/issues/1045), [#812](https://github.com/evanw/esbuild/issues/812), [#661](https://github.com/evanw/esbuild/issues/661), [#574](https://github.com/evanw/esbuild/issues/574), [#512](https://github.com/evanw/esbuild/issues/512), [#495](https://github.com/evanw/esbuild/issues/495), [#480](https://github.com/evanw/esbuild/issues/480), [#453](https://github.com/evanw/esbuild/issues/453), [#410](https://github.com/evanw/esbuild/issues/410), [#80](https://github.com/evanw/esbuild/issues/80)) + + Previously esbuild had warnings when bundling about uses of `require` and `import` that are not of the form `require()` or `import()`. These warnings existed because the bundling process must be able to statically-analyze all dynamic imports to determine which files must be included. Here are some real-world examples of cases that esbuild doesn't statically analyze: + + * From [`mongoose`](https://www.npmjs.com/package/mongoose): + + ```js + require('./driver').set(require(global.MONGOOSE_DRIVER_PATH)); + ``` + + * From [`moment`](https://www.npmjs.com/package/moment): + + ```js + aliasedRequire = require; + aliasedRequire('./locale/' + name); + ``` + + * From [`logform`](https://www.npmjs.com/package/logform): + + ```js + function exposeFormat(name) { + Object.defineProperty(format, name, { + get() { return require(`./${name}.js`); } + }); + } + exposeFormat('align'); + ``` + + All of these dynamic imports will not be bundled (i.e. they will be left as-is) and will crash at run-time if they are evaluated. Some of these crashes are ok since the code paths may have error handling or the code paths may never be used. Other crashes are not ok because the crash will actually be hit. + + The warning from esbuild existed to let you know that esbuild is aware that it's generating a potentially broken bundle. If you discover that your bundle is broken, it's nice to have a warning from esbuild to point out where the problem is. And it was just a warning so the build process still finishes and successfully generates output files. If you didn't want to see the warning, it was easy to turn it off via `--log-level=error`. + + However, there have been quite a few complaints about this warning. Some people seem to not understand the difference between a warning and an error, and think the build has failed even though output files were generated. Other people do not want to see the warning but also do not want to enable `--log-level=error`. + + This release removes this warning for both `require` and `import`. Now when you try to bundle code with esbuild that contains dynamic imports not of the form `require()` or `import()`, esbuild will just silently generate a potentially broken bundle. This may affect people coming from other bundlers that support certain forms of dynamic imports that are not compatible with esbuild such as the [Webpack-specific dynamic `import()` with pattern matching](https://webpack.js.org/api/module-methods/#dynamic-expressions-in-import). + ## 0.11.10 * Provide more information about `exports` map import failures if possible ([#1143](https://github.com/evanw/esbuild/issues/1143)) diff --git a/internal/ast/ast.go b/internal/ast/ast.go index 06117b8bb61..0de1a910c3f 100644 --- a/internal/ast/ast.go +++ b/internal/ast/ast.go @@ -88,9 +88,17 @@ type ImportRecord struct { // Tell the printer to wrap this call to "require()" in "__toModule(...)" WrapWithToModule bool - // True for require calls like this: "try { require() } catch {}". In this - // case we shouldn't generate an error if the path could not be resolved. - IsInsideTryBody bool + // True for the following cases: + // + // try { require('x') } catch { handle } + // try { await import('x') } catch { handle } + // try { require.resolve('x') } catch { handle } + // import('x').catch(handle) + // import('x').then(_, handle) + // + // In these cases we shouldn't generate an error if the path could not be + // resolved. + HandlesImportErrors bool // If true, this was originally written as a bare "import 'file'" statement WasOriginallyBareImport bool diff --git a/internal/bundler/bundler.go b/internal/bundler/bundler.go index 16a99dece25..e7793d8bc63 100644 --- a/internal/bundler/bundler.go +++ b/internal/bundler/bundler.go @@ -404,7 +404,7 @@ func parseFile(args parseArgs) { // All "require.resolve()" imports should be external because we don't // want to waste effort traversing into them if record.Kind == ast.ImportRequireResolve { - if !record.IsInsideTryBody && (resolveResult == nil || !resolveResult.IsExternal) { + if !record.HandlesImportErrors && (resolveResult == nil || !resolveResult.IsExternal) { args.log.AddRangeWarning(&source, record.Range, fmt.Sprintf("%q should be marked as external for use with \"require.resolve\"", record.Path.Text)) } @@ -416,10 +416,15 @@ func parseFile(args parseArgs) { // external imports instead of causing errors. This matches a common // code pattern for conditionally importing a module with a graceful // fallback. - if !didLogError && !record.IsInsideTryBody { + if !didLogError && !record.HandlesImportErrors { hint := "" if resolver.IsPackagePath(record.Path.Text) { - hint = " (mark it as external to exclude it from the bundle)" + if record.Kind == ast.ImportRequire { + hint = ", or surround it with try/catch to handle the failure at run-time" + } else if record.Kind == ast.ImportDynamic { + hint = ", or add \".catch()\" to handle the failure at run-time" + } + hint = fmt.Sprintf(" (mark it as external to exclude it from the bundle%s)", hint) if pluginName == "" && !args.fs.IsAbs(record.Path.Text) { if query := args.res.ProbeResolvePackageAsRelative(absResolveDir, record.Path.Text, record.Kind); query != nil { hint = fmt.Sprintf(" (use %q to reference the file %q)", "./"+record.Path.Text, args.res.PrettyPath(query.PathPair.Primary)) diff --git a/internal/bundler/bundler_default_test.go b/internal/bundler/bundler_default_test.go index 94452bab94c..d95c58530df 100644 --- a/internal/bundler/bundler_default_test.go +++ b/internal/bundler/bundler_default_test.go @@ -821,12 +821,6 @@ 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 (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 (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) `, }) } @@ -940,8 +934,6 @@ func TestConditionalImport(t *testing.T) { }, }, }, - 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) -`, }) } @@ -1056,8 +1048,6 @@ func TestRequireWithoutCall(t *testing.T) { Mode: config.ModeBundle, AbsOutputFile: "/out.js", }, - expectedScanLog: `entry.js: warning: Indirect calls to "require" will not be bundled (surround with a try/catch to silence this warning) -`, }) } @@ -1076,8 +1066,6 @@ func TestNestedRequireWithoutCall(t *testing.T) { Mode: config.ModeBundle, AbsOutputFile: "/out.js", }, - expectedScanLog: `entry.js: warning: Indirect calls to "require" will not be bundled (surround with a try/catch to silence this warning) -`, }) } @@ -1144,31 +1132,6 @@ func TestRequirePropertyAccessCommonJS(t *testing.T) { }) } -func TestRequirePropertyAccessES6(t *testing.T) { - default_suite.expectBundled(t, bundled{ - files: map[string]string{ - "/entry.js": ` - // These should warn since the format is ESM - console.log(Object.keys(require.cache)) - console.log(Object.keys(require.extensions)) - delete require.cache['fs'] - delete require.extensions['.json'] - `, - }, - entryPaths: []string{"/entry.js"}, - options: config.Options{ - Mode: config.ModeBundle, - OutputFormat: config.FormatESModule, - AbsOutputFile: "/out.js", - }, - 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) -`, - }) -} - // Test a workaround for code using "await import()" func TestAwaitImportInsideTry(t *testing.T) { default_suite.expectBundled(t, bundled{ @@ -1195,13 +1158,12 @@ func TestImportInsideTry(t *testing.T) { default_suite.expectBundled(t, bundled{ files: map[string]string{ "/entry.js": ` - async function main(name) { - try { - return import(name) - } catch { - } + let x + try { + x = import('nope1') + x = await import('nope2') + } catch { } - main('fs') `, }, entryPaths: []string{"/entry.js"}, @@ -1209,7 +1171,7 @@ 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 (use "import().catch()" to silence this warning) + expectedScanLog: `entry.js: error: Could not resolve "nope1" (mark it as external to exclude it from the bundle, or add ".catch()" to handle the failure at run-time) `, }) } @@ -1314,83 +1276,6 @@ func TestHashbangNoBundle(t *testing.T) { }) } -func TestTypeofRequireBundle(t *testing.T) { - default_suite.expectBundled(t, bundled{ - files: map[string]string{ - "/entry.js": ` - console.log([ - typeof require, - typeof require == 'function', - typeof require == 'function' && require, - 'function' == typeof require, - 'function' == typeof require && require, - ]); - `, - }, - entryPaths: []string{"/entry.js"}, - options: config.Options{ - Mode: config.ModeBundle, - AbsOutputFile: "/out.js", - }, - }) -} - -func TestTypeofRequireNoBundle(t *testing.T) { - default_suite.expectBundled(t, bundled{ - files: map[string]string{ - "/entry.js": ` - console.log([ - typeof require, - typeof require == 'function', - typeof require == 'function' && require, - 'function' == typeof require, - 'function' == typeof require && require, - ]); - `, - }, - entryPaths: []string{"/entry.js"}, - options: config.Options{ - AbsOutputFile: "/out.js", - }, - }) -} - -func TestTypeofRequireBadPatterns(t *testing.T) { - default_suite.expectBundled(t, bundled{ - files: map[string]string{ - "/entry.js": ` - console.log([ - typeof require != 'function' && require, - typeof require === 'function' && require, - typeof require == 'function' || require, - typeof require == 'function' && notRequire, - typeof notRequire == 'function' && require, - - 'function' != typeof require && require, - 'function' === typeof require && require, - 'function' == typeof require || require, - 'function' == typeof require && notRequire, - 'function' == typeof notRequire && require, - ]); - `, - }, - entryPaths: []string{"/entry.js"}, - options: config.Options{ - Mode: config.ModeBundle, - AbsOutputFile: "/out.js", - }, - 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) -`, - }) -} - func TestRequireFSBrowser(t *testing.T) { default_suite.expectBundled(t, bundled{ files: map[string]string{ @@ -3559,10 +3444,7 @@ func TestRequireResolve(t *testing.T) { }, }, }, - 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: "./present-file" should be marked as external for use with "require.resolve" + expectedScanLog: `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" @@ -4114,26 +3996,6 @@ func TestRequireMainCacheCommonJS(t *testing.T) { }) } -func TestRequireMainCacheIIFE(t *testing.T) { - default_suite.expectBundled(t, bundled{ - files: map[string]string{ - "/entry.js": ` - console.log('is main:', require.main === module) - console.log('cache:', require.cache); - `, - }, - entryPaths: []string{"/entry.js"}, - options: config.Options{ - Mode: config.ModeBundle, - AbsOutputFile: "/out.js", - OutputFormat: config.FormatIIFE, - }, - 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) -`, - }) -} - func TestExternalES6ConvertedToCommonJS(t *testing.T) { default_suite.expectBundled(t, bundled{ files: map[string]string{ diff --git a/internal/bundler/bundler_packagejson_test.go b/internal/bundler/bundler_packagejson_test.go index ff8bdee8a26..110c8559a15 100644 --- a/internal/bundler/bundler_packagejson_test.go +++ b/internal/bundler/bundler_packagejson_test.go @@ -1879,11 +1879,11 @@ func TestPackageJsonExportsMustUseImport(t *testing.T) { Mode: config.ModeBundle, AbsOutputFile: "/Users/user/project/out.js", }, - expectedScanLog: `Users/user/project/src/entry.js: error: Could not resolve "pkg1" (mark it as external to exclude it from the bundle) + expectedScanLog: `Users/user/project/src/entry.js: error: Could not resolve "pkg1" (mark it as external to exclude it from the bundle, or surround it with try/catch to handle the failure at run-time) Users/user/project/node_modules/pkg1/package.json: note: The path "." is not currently exported by package "pkg1" Users/user/project/node_modules/pkg1/package.json: note: None of the conditions provided ("import") match any of the currently active conditions ("browser", "default", "require") Users/user/project/src/entry.js: note: Consider using an "import" statement to import this file -Users/user/project/src/entry.js: error: Could not resolve "pkg1/foo.js" (mark it as external to exclude it from the bundle) +Users/user/project/src/entry.js: error: Could not resolve "pkg1/foo.js" (mark it as external to exclude it from the bundle, or surround it with try/catch to handle the failure at run-time) Users/user/project/node_modules/pkg1/package.json: note: The path "./foo.js" is not currently exported by package "pkg1" Users/user/project/node_modules/pkg1/package.json: note: None of the conditions provided ("import") match any of the currently active conditions ("browser", "default", "require") Users/user/project/src/entry.js: note: Consider using an "import" statement to import this file @@ -1918,11 +1918,11 @@ func TestPackageJsonExportsReverseLookup(t *testing.T) { Mode: config.ModeBundle, AbsOutputFile: "/Users/user/project/out.js", }, - expectedScanLog: `Users/user/project/src/entry.js: error: Could not resolve "pkg/path/to/real/file" (mark it as external to exclude it from the bundle) + expectedScanLog: `Users/user/project/src/entry.js: error: Could not resolve "pkg/path/to/real/file" (mark it as external to exclude it from the bundle, or surround it with try/catch to handle the failure at run-time) Users/user/project/node_modules/pkg/package.json: note: The path "./path/to/real/file" is not exported by package "pkg" Users/user/project/node_modules/pkg/package.json: note: The file "./path/to/real/file.js" is exported at path "./lib/teal/file" Users/user/project/src/entry.js: note: Import from "pkg/lib/teal/file" to get the file "Users/user/project/node_modules/pkg/path/to/real/file.js" -Users/user/project/src/entry.js: error: Could not resolve "pkg/path/to/other/file" (mark it as external to exclude it from the bundle) +Users/user/project/src/entry.js: error: Could not resolve "pkg/path/to/other/file" (mark it as external to exclude it from the bundle, or surround it with try/catch to handle the failure at run-time) Users/user/project/node_modules/pkg/package.json: note: The path "./path/to/other/file" is not exported by package "pkg" Users/user/project/node_modules/pkg/package.json: note: The file "./path/to/other/file.js" is exported at path "./extra/other/file.js" Users/user/project/src/entry.js: note: Import from "pkg/extra/other/file.js" to get the file "Users/user/project/node_modules/pkg/path/to/other/file.js" diff --git a/internal/bundler/snapshots/snapshots_default.txt b/internal/bundler/snapshots/snapshots_default.txt index bcc87c3ef6d..bd1376da96f 100644 --- a/internal/bundler/snapshots/snapshots_default.txt +++ b/internal/bundler/snapshots/snapshots_default.txt @@ -948,18 +948,6 @@ const imp = [ ]; console.log(ns, a, c, def, def2, ns2, def3, a2, c3, imp); -================================================================================ -TestImportInsideTry ----------- /out.js ---------- -// entry.js -async function main(name) { - try { - return import(name); - } catch { - } -} -main("fs"); - ================================================================================ TestImportMetaCommonJS ---------- /out.js ---------- @@ -2126,18 +2114,6 @@ console.log("is main:", require.main === module); console.log(require_is_main()); console.log("cache:", require.cache); -================================================================================ -TestRequireMainCacheIIFE ----------- /out.js ---------- -(() => { - // entry.js - var require_entry = __commonJS((exports, module) => { - console.log("is main:", require.main === module); - console.log("cache:", require.cache); - }); - require_entry(); -})(); - ================================================================================ TestRequireParentDirCommonJS ---------- /out.js ---------- @@ -2167,15 +2143,6 @@ console.log(Object.keys(require.extensions)); delete require.cache["fs"]; delete require.extensions[".json"]; -================================================================================ -TestRequirePropertyAccessES6 ----------- /out.js ---------- -// entry.js -console.log(Object.keys(require.cache)); -console.log(Object.keys(require.extensions)); -delete require.cache["fs"]; -delete require.extensions[".json"]; - ================================================================================ TestRequireResolve ---------- /out.js ---------- @@ -2930,46 +2897,6 @@ TestTopLevelReturnAllowedImport return; import "foo"; -================================================================================ -TestTypeofRequireBadPatterns ----------- /out.js ---------- -// entry.js -console.log([ - false, - require, - true, - notRequire, - typeof notRequire == "function" && require, - false, - require, - true, - notRequire, - typeof notRequire == "function" && require -]); - -================================================================================ -TestTypeofRequireBundle ----------- /out.js ---------- -// entry.js -console.log([ - "function", - true, - false, - true, - false -]); - -================================================================================ -TestTypeofRequireNoBundle ----------- /out.js ---------- -console.log([ - typeof require, - typeof require == "function", - typeof require == "function" && require, - typeof require == "function", - typeof require == "function" && require -]); - ================================================================================ TestUseStrictDirectiveMinifyNoBundle ---------- /out.js ---------- diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go index dbc81ccdb57..40c8f9eb4d1 100644 --- a/internal/js_parser/js_parser.go +++ b/internal/js_parser/js_parser.go @@ -143,69 +143,6 @@ type parser struct { moduleScope *js_ast.Scope isControlFlowDead bool - // These are for recognizing "typeof require == 'function' && require". This - // is a workaround for code that browserify generates that looks like this: - // - // (function e(t, n, r) { - // function s(o2, u) { - // if (!n[o2]) { - // if (!t[o2]) { - // var a = typeof require == "function" && require; - // if (!u && a) - // return a(o2, true); - // if (i) - // return i(o2, true); - // throw new Error("Cannot find module '" + o2 + "'"); - // } - // var f = n[o2] = {exports: {}}; - // t[o2][0].call(f.exports, function(e2) { - // var n2 = t[o2][1][e2]; - // return s(n2 ? n2 : e2); - // }, f, f.exports, e, t, n, r); - // } - // return n[o2].exports; - // } - // var i = typeof require == "function" && require; - // for (var o = 0; o < r.length; o++) - // s(r[o]); - // return s; - // }); - // - // It's checking to see if the environment it's running in has a "require" - // function before calling it. However, esbuild's bundling environment has a - // bundle-time require function because it's a bundler. So in this case - // "typeof require == 'function'" is true and the "&&" expression just - // becomes a single "require" identifier, which will then crash at run time. - // - // The workaround is to explicitly pattern-match for the exact expression - // "typeof require == 'function' && require" and replace it with "false" if - // we're targeting the browser. - // - // Note that we can't just leave "typeof require == 'function'" alone because - // there is other code in the wild that legitimately does need it to become - // "true" when bundling. Specifically, the package "@dagrejs/graphlib" has - // code that looks like this: - // - // if (typeof require === "function") { - // try { - // lodash = { - // clone: require("lodash/clone"), - // constant: require("lodash/constant"), - // each: require("lodash/each"), - // // ... more calls to require() here ... - // }; - // } catch (e) { - // // continue regardless of error - // } - // } - // - // That library will crash later on during startup if that branch isn't - // taken because "typeof require === 'function'" is false at run time. - typeofTarget js_ast.E - typeofRequire js_ast.E - typeofRequireEqualsFn js_ast.E - typeofRequireEqualsFnTarget js_ast.E - // This helps recognize the "await import()" pattern. When this is present, // warnings about non-string import paths will be omitted inside try blocks. awaitTarget js_ast.E @@ -214,15 +151,6 @@ type parser struct { // 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". - cjsDotTarget 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 @@ -420,19 +348,9 @@ type fnOrArrowDataVisit struct { isInsideSwitch bool isOutsideFnOrArrow bool - // This is used to silence references to "require" inside a try/catch - // statement. The assumption is that the try/catch statement is there to - // handle the case where the reference to "require" crashes. Specifically, - // the workaround handles the "moment" library which contains code that - // looks like this: - // - // try { - // oldLocale = globalLocale._abbr; - // var aliasedRequire = require; - // aliasedRequire('./locale/' + name); - // getSetGlobalLocale(oldLocale); - // } catch (e) {} - // + // This is used to silence unresolvable imports due to "require" calls inside + // a try/catch statement. The assumption is that the try/catch statement is + // there to handle the case where the reference to "require" crashes. tryBodyCount int } @@ -10044,11 +9962,6 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO wasAnonymousNamedExpr := p.isAnonymousNamedExpr(e.Right) e.Left, _ = p.visitExprInOut(e.Left, exprIn{assignTarget: e.Op.BinaryAssignTarget()}) - // Pattern-match "typeof require == 'function' && ___" from browserify - if e.Op == js_ast.BinOpLogicalAnd && e.Left.Data == p.typeofRequireEqualsFn { - p.typeofRequireEqualsFnTarget = e.Right.Data - } - // Mark the control flow as dead if the branch is never taken switch e.Op { case js_ast.BinOpLogicalOr: @@ -10118,17 +10031,7 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO case js_ast.BinOpLooseEq: if result, ok := checkEqualityIfNoSideEffects(e.Left.Data, e.Right.Data); ok { - data := &js_ast.EBoolean{Value: result} - - // Pattern-match "typeof require == 'function'" from browserify. Also - // match "'function' == typeof require" because some minifiers such as - // terser transpose the left and right operands to "==" to form a - // different but equivalent expression. - if result && (e.Left.Data == p.typeofRequire || e.Right.Data == p.typeofRequire) { - p.typeofRequireEqualsFn = data - } - - return js_ast.Expr{Loc: expr.Loc, Data: data}, exprOut{} + return js_ast.Expr{Loc: expr.Loc, Data: &js_ast.EBoolean{Value: result}}, exprOut{} } afterOpLoc := locAfterOp(e) if !p.warnAboutEqualityCheck("==", e.Left, afterOpLoc) { @@ -10622,8 +10525,6 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO case *js_ast.EUnary: switch e.Op { case js_ast.UnOpTypeof: - p.typeofTarget = e.Value.Data - _, idBefore := e.Value.Data.(*js_ast.EIdentifier) e.Value, _ = p.visitExprInOut(e.Value, exprIn{assignTarget: e.Op.UnaryAssignTarget()}) id, idAfter := e.Value.Data.(*js_ast.EIdentifier) @@ -10634,15 +10535,6 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO e.Value = js_ast.JoinWithComma(js_ast.Expr{Loc: e.Value.Loc, Data: &js_ast.ENumber{}}, e.Value) } - // "typeof require" => "'function'" - if p.options.mode == config.ModeBundle { - if id, ok := e.Value.Data.(*js_ast.EIdentifier); ok && id.Ref == p.requireRef { - p.ignoreUsage(p.requireRef) - p.typeofRequire = &js_ast.EString{Value: js_lexer.StringToUTF16("function")} - return js_ast.Expr{Loc: expr.Loc, Data: p.typeofRequire}, exprOut{} - } - } - if typeof, ok := typeofWithoutSideEffects(e.Value.Data); ok { return js_ast.Expr{Loc: expr.Loc, Data: &js_ast.EString{Value: js_lexer.StringToUTF16(typeof)}}, exprOut{} } @@ -10797,11 +10689,6 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO } } - // This helps us pattern-match "require.someProperty" when targeting CommonJS - if p.options.outputFormat == config.FormatCommonJS { - p.cjsDotTarget = e.Target.Data - } - // Track ".then().catch()" chains if isCallTarget && p.thenCatchChain.nextTarget == e { if e.Name == "catch" { @@ -11127,6 +11014,7 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO } importRecordIndex := p.addImportRecord(ast.ImportDynamic, arg.Loc, js_lexer.UTF16ToString(str.Value)) + p.importRecords[importRecordIndex].HandlesImportErrors = (isAwaitTarget && p.fnOrArrowDataVisit.tryBodyCount != 0) || isThenCatchTarget p.importRecordsForCurrentPart = append(p.importRecordsForCurrentPart, importRecordIndex) return js_ast.Expr{Loc: expr.Loc, Data: &js_ast.EImport{ Expr: arg, @@ -11135,26 +11023,6 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO }} } - if p.options.mode == config.ModeBundle { - // 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) - } - } - // We need to convert this into a call to "require()" if ES6 syntax is // not supported in the current output format. The full conversion: // @@ -11218,7 +11086,6 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO couldBeRequireResolve := false if len(e.Args) == 1 && p.options.mode != config.ModePassThrough { if dot, ok := e.Target.Data.(*js_ast.EDot); ok && dot.OptionalChain == js_ast.OptionalChainNone && dot.Name == "resolve" { - p.resolveCallTarget = dot.Target.Data couldBeRequireResolve = true } } @@ -11267,7 +11134,7 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO } importRecordIndex := p.addImportRecord(ast.ImportRequireResolve, e.Args[0].Loc, js_lexer.UTF16ToString(str.Value)) - p.importRecords[importRecordIndex].IsInsideTryBody = p.fnOrArrowDataVisit.tryBodyCount != 0 + p.importRecords[importRecordIndex].HandlesImportErrors = p.fnOrArrowDataVisit.tryBodyCount != 0 p.importRecordsForCurrentPart = append(p.importRecordsForCurrentPart, importRecordIndex) // Create a new expression to represent the operation @@ -11393,7 +11260,7 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO } importRecordIndex := p.addImportRecord(ast.ImportRequire, arg.Loc, js_lexer.UTF16ToString(str.Value)) - p.importRecords[importRecordIndex].IsInsideTryBody = p.fnOrArrowDataVisit.tryBodyCount != 0 + p.importRecords[importRecordIndex].HandlesImportErrors = p.fnOrArrowDataVisit.tryBodyCount != 0 p.importRecordsForCurrentPart = append(p.importRecordsForCurrentPart, importRecordIndex) // Create a new expression to represent the operation @@ -11646,24 +11513,6 @@ func (p *parser) handleIdentifier(loc logger.Loc, e *js_ast.EIdentifier, opts id } } - // Warn about uses of "require" other than a direct "require()" call, a - // "typeof require" expression, or a "require.someProperty" access. But - // suppress warnings inside a try body block since presumably the try/catch - // is there to handle run-time failures due to indirect require calls. - if ref == p.requireRef && e != p.callTarget && e != p.typeofTarget && e != p.cjsDotTarget && p.fnOrArrowDataVisit.tryBodyCount == 0 { - // "typeof require == 'function' && require" - if e == p.typeofRequireEqualsFnTarget { - // Become "false" in the browser and "require" in node - if p.options.platform == config.PlatformBrowser { - return js_ast.Expr{Loc: loc, Data: &js_ast.EBoolean{Value: false}} - } - } 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 (surround with a try/catch to silence this warning)") - } - } - return js_ast.Expr{Loc: loc, Data: e} } From 60c58e12887162b6e4f41ca81d2d674e5d01b2ce Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Wed, 14 Apr 2021 18:20:37 -0700 Subject: [PATCH 2/2] debug a weird ci issue --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 8a6d7ee9fa5..66e337cfc46 100644 --- a/Makefile +++ b/Makefile @@ -32,7 +32,7 @@ vet-go: go vet ./cmd/... ./internal/... ./pkg/... fmt-go: - go fmt ./cmd/... ./internal/... ./pkg/... + go fmt -x ./cmd/... ./internal/... ./pkg/... no-filepath: @! grep --color --include '*.go' -r '"path/filepath"' cmd internal pkg || ( \