Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

remove "require" and "import" warnings #1155

Merged
merged 2 commits into from
Apr 15, 2021
Merged

remove "require" and "import" warnings #1155

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8fe9e1c
remove "require" and "import" warnings
evanw Apr 15, 2021
60c58e1
debug a weird ci issue
evanw Apr 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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(<string literal>)` or `import(<string literal>)`. 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(<string literal>)` or `import(<string literal>)`, 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))
Expand Down
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 || ( \
Expand Down
14 changes: 11 additions & 3 deletions internal/ast/ast.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 8 additions & 3 deletions internal/bundler/bundler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
Expand All @@ -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))
Expand Down
152 changes: 7 additions & 145 deletions internal/bundler/bundler_default_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
`,
})
}
Expand Down Expand Up @@ -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)
`,
})
}

Expand Down Expand Up @@ -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)
`,
})
}

Expand All @@ -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)
`,
})
}

Expand Down Expand Up @@ -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{
Expand All @@ -1195,21 +1158,20 @@ 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"},
options: config.Options{
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)
`,
})
}
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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{
Expand Down
8 changes: 4 additions & 4 deletions internal/bundler/bundler_packagejson_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"
Expand Down
Loading