From 41d16b90b2c2b7a1436688de2d708cbbb8541f37 Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Tue, 16 Feb 2021 10:37:54 +1100 Subject: [PATCH 1/3] Stop indirect call warning for 'require.cache' for CommonJS modules --- internal/bundler/bundler_default_test.go | 7 +++++-- internal/bundler/snapshots/snapshots_default.txt | 6 ++++-- internal/js_parser/js_parser.go | 16 ++++++++-------- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/internal/bundler/bundler_default_test.go b/internal/bundler/bundler_default_test.go index c72c77ae984..f4de7e5b88f 100644 --- a/internal/bundler/bundler_default_test.go +++ b/internal/bundler/bundler_default_test.go @@ -3793,12 +3793,13 @@ func TestConstWithLetNoMangle(t *testing.T) { }) } -func TestRequireMainCommonJS(t *testing.T) { +func TestRequireMainCacheCommonJS(t *testing.T) { default_suite.expectBundled(t, bundled{ files: map[string]string{ "/entry.js": ` console.log('is main:', require.main === module) console.log(require('./is-main')) + console.log('cache:', require.cache); `, "/is-main.js": ` module.exports = require.main === module @@ -3813,11 +3814,12 @@ func TestRequireMainCommonJS(t *testing.T) { }) } -func TestRequireMainIIFE(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"}, @@ -3827,6 +3829,7 @@ func TestRequireMainIIFE(t *testing.T) { 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) `, }) } diff --git a/internal/bundler/snapshots/snapshots_default.txt b/internal/bundler/snapshots/snapshots_default.txt index 4ff138d604a..7dff2a997c9 100644 --- a/internal/bundler/snapshots/snapshots_default.txt +++ b/internal/bundler/snapshots/snapshots_default.txt @@ -1944,7 +1944,7 @@ var require_test = __commonJS((exports, module) => { console.log(require_test()); ================================================================================ -TestRequireMainCommonJS +TestRequireMainCacheCommonJS ---------- /out.js ---------- // is-main.js var require_is_main = __commonJS((exports2, module2) => { @@ -1954,14 +1954,16 @@ var require_is_main = __commonJS((exports2, module2) => { // entry.js console.log("is main:", require.main === module); console.log(require_is_main()); +console.log("cache:", require.cache); ================================================================================ -TestRequireMainIIFE +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(); })(); diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go index 2eb44da78a7..f0e38687f40 100644 --- a/internal/js_parser/js_parser.go +++ b/internal/js_parser/js_parser.go @@ -206,10 +206,10 @@ type parser struct { // warnings about non-string import paths will be omitted inside try blocks. awaitTarget js_ast.E - // This helps recognize the "require.main" pattern. If this pattern is - // present and the output format is CommonJS, we avoid generating a warning - // about an unbundled use of "require". - cjsDotMainTarget js_ast.E + // These helps recognize the "require.main" and "require.cache" patterns. If + // this pattern is present and the output format is CommonJS, we avoid + // generating a warning about an unbundled use of "require". + cjsDotMainOrCacheTarget js_ast.E // This helps recognize calls to "require.resolve()" which may become // ERequireResolve expressions. @@ -10392,9 +10392,9 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO } } - // Pattern-match "require.main" from node - if p.options.outputFormat == config.FormatCommonJS && e.Name == "main" { - p.cjsDotMainTarget = e.Target.Data + // Pattern-match "require.main" and "require.cache" from node + if p.options.outputFormat == config.FormatCommonJS && (e.Name == "main" || e.Name == "cache") { + p.cjsDotMainOrCacheTarget = e.Target.Data } isCallTarget := e == p.callTarget @@ -11157,7 +11157,7 @@ func (p *parser) handleIdentifier(loc logger.Loc, assignTarget js_ast.AssignTarg } // Warn about uses of "require" other than a direct call - if ref == p.requireRef && e != p.callTarget && e != p.typeofTarget && e != p.cjsDotMainTarget && p.fnOrArrowDataVisit.tryBodyCount == 0 { + if ref == p.requireRef && e != p.callTarget && e != p.typeofTarget && e != p.cjsDotMainOrCacheTarget && p.fnOrArrowDataVisit.tryBodyCount == 0 { // "typeof require == 'function' && require" if e == p.typeofRequireEqualsFnTarget { // Become "false" in the browser and "require" in node From a7dcc0d120f1c0496afd5bb90922b5626762f5dc Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Tue, 16 Feb 2021 10:45:23 +1100 Subject: [PATCH 2/3] Add CHANGELOG entry --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 61f8f1dc797..e785213f1cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +* Omit a warning about `require.cache` when targeting CommonJS ([#812](https://github.com/evanw/esbuild/issues/812)) + + The `require.cache` property allows introspecting the state of the `require` cache, generally without affecting what is imported/bundled. Previously esbuild generated a warning about an unexpected use of `require`. Now this warning is no longer generated for `require.cache` when the output format is `cjs`. + ## 0.8.46 * Fix minification of `.0` in CSS ([#804](https://github.com/evanw/esbuild/issues/804)) From 3dcc9aa56e27dcb9dc8d6c3f2f6bb06e85546e64 Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Mon, 15 Feb 2021 21:27:57 -0800 Subject: [PATCH 3/3] ignore all "require.someProp" warnings --- CHANGELOG.md | 8 +++- internal/bundler/bundler_default_test.go | 45 +++++++++++++++++++ .../bundler/snapshots/snapshots_default.txt | 18 ++++++++ internal/js_parser/js_parser.go | 21 +++++---- 4 files changed, 81 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e785213f1cc..1f5bc01f437 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,9 +2,13 @@ ## Unreleased -* Omit a warning about `require.cache` when targeting CommonJS ([#812](https://github.com/evanw/esbuild/issues/812)) +* Omit warning about `require.someProperty` when targeting CommonJS ([#812](https://github.com/evanw/esbuild/issues/812)) - The `require.cache` property allows introspecting the state of the `require` cache, generally without affecting what is imported/bundled. Previously esbuild generated a warning about an unexpected use of `require`. Now this warning is no longer generated for `require.cache` when the output format is `cjs`. + The `require.cache` property allows introspecting the state of the `require` cache, generally without affecting what is imported/bundled. + + Since esbuild's static analyzer only detects direct calls to `require`, it currently warns about uses of `require` in any situation other than a direct call since that means the value is "escaping" the analyzer. This is meant to detect and warn about indirect calls such as `['fs', 'path'].map(require)`. + + However, this warning is not relevant when accessing a property off of the `require` object such as `require.cache` because a property access does not result in capturing the value of `require`. Now a warning is no longer generated for `require.someProperty` when the output format is `cjs`. This allows for the use of features such as `require.cache` and `require.extensions`. ## 0.8.46 diff --git a/internal/bundler/bundler_default_test.go b/internal/bundler/bundler_default_test.go index f4de7e5b88f..3db3a290a9f 100644 --- a/internal/bundler/bundler_default_test.go +++ b/internal/bundler/bundler_default_test.go @@ -1185,6 +1185,51 @@ func TestRequireWithoutCallInsideTry(t *testing.T) { }) } +func TestRequirePropertyAccessCommonJS(t *testing.T) { + default_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/entry.js": ` + // These shouldn't warn + 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.FormatCommonJS, + AbsOutputFile: "/out.js", + }, + }) +} + +func TestRequirePropertyAccessES6(t *testing.T) { + default_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/entry.js": ` + // These shouldn't warn + 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{ diff --git a/internal/bundler/snapshots/snapshots_default.txt b/internal/bundler/snapshots/snapshots_default.txt index 7dff2a997c9..e09a10ed9c0 100644 --- a/internal/bundler/snapshots/snapshots_default.txt +++ b/internal/bundler/snapshots/snapshots_default.txt @@ -1988,6 +1988,24 @@ var src_default = 123; // Users/user/project/src/dir/entry.js console.log(src_default); +================================================================================ +TestRequirePropertyAccessCommonJS +---------- /out.js ---------- +// entry.js +console.log(Object.keys(require.cache)); +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 ---------- diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go index f0e38687f40..7f09511016f 100644 --- a/internal/js_parser/js_parser.go +++ b/internal/js_parser/js_parser.go @@ -206,10 +206,10 @@ type parser struct { // warnings about non-string import paths will be omitted inside try blocks. awaitTarget js_ast.E - // These helps recognize the "require.main" and "require.cache" patterns. If - // this pattern is present and the output format is CommonJS, we avoid - // generating a warning about an unbundled use of "require". - cjsDotMainOrCacheTarget js_ast.E + // 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. @@ -10392,9 +10392,9 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO } } - // Pattern-match "require.main" and "require.cache" from node - if p.options.outputFormat == config.FormatCommonJS && (e.Name == "main" || e.Name == "cache") { - p.cjsDotMainOrCacheTarget = e.Target.Data + // This helps us pattern-match "require.someProperty" when targeting CommonJS + if p.options.outputFormat == config.FormatCommonJS { + p.cjsDotTarget = e.Target.Data } isCallTarget := e == p.callTarget @@ -11156,8 +11156,11 @@ func (p *parser) handleIdentifier(loc logger.Loc, assignTarget js_ast.AssignTarg } } - // Warn about uses of "require" other than a direct call - if ref == p.requireRef && e != p.callTarget && e != p.typeofTarget && e != p.cjsDotMainOrCacheTarget && p.fnOrArrowDataVisit.tryBodyCount == 0 { + // 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