fix #999: no "sideEffects" warn in "node_modules"

CHANGELOG.md

@@ -57,6 +57,12 @@
 
     This release changes the install script for the `esbuild` npm package to use the "rename a temporary file" approach instead of the "write the file directly" approach to replace the `esbuild` command stub file with the real binary executable. This should hopefully work around a problem with the [pnpm](https://pnpm.js.org/) package manager and its use of hard links.
 
+* Avoid warning about potential issues with `sideEffects` in packages ([#999](https://github.com/evanw/esbuild/issues/999))
+
+    Bare imports such as `import "foo"` mean the package is only imported for its side effects. Doing this when the package contains `"sideEffects": false` in `package.json` causes a warning because it means esbuild will not import the file since it has been marked as having no side effects, even though the import statement clearly expects it to have side effects. This is usually caused by an incorrect `sideEffects` annotation in the package.
+
+    However, this warning is not immediately actionable if the file containing the import statement is itself in a package. So with this release, esbuild will no longer issue this warning if the file containing the import is inside a `node_modules` folder. Note that even though the warning is no longer there, this situation can still result in a broken bundle if the `sideEffects` annotation is incorrect.
+
 ## 0.9.3
 
 * Fix path resolution with the `exports` field for scoped packages

internal/bundler/bundler.go

@@ -1434,9 +1434,16 @@ func (s *scanner) processScannedFiles() []file {
 					}
 				}
 
-				// Don't include this module for its side effects if it can be
-				// considered to have no side effects
-				if record.WasOriginallyBareImport && !s.options.IgnoreDCEAnnotations {
+				// Warn about this import if it's a bare import statement without any
+				// imported names (i.e. a side-effect-only import) and the module has
+				// been marked as having no side effects.
+				//
+				// Except don't do this if this file is inside "node_modules" since
+				// it's a bug in the package and the user won't be able to do anything
+				// about it. Note that this can result in esbuild silently generating
+				// broken code. If this actually happens for people, it's probably worth
+				// re-enabling the warning about code inside "node_modules".
+				if record.WasOriginallyBareImport && !s.options.IgnoreDCEAnnotations && !resolver.IsInsideNodeModules(result.file.source.KeyPath.Text) {
 					if otherFile := &s.results[record.SourceIndex.GetIndex()].file; otherFile.warnIfUnused {
 						var notes []logger.MsgData
 						if otherFile.warnIfUnusedData != nil {

internal/bundler/bundler_dce_test.go

@@ -779,6 +779,35 @@ func TestPackageJsonSideEffectsKeepExportDefaultExpr(t *testing.T) {
 	})
 }
 
+func TestPackageJsonSideEffectsFalseNoWarningInNodeModulesIssue999(t *testing.T) {
+	dce_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/Users/user/project/src/entry.js": `
+				import "demo-pkg"
+				console.log('used import')
+				`,
+			"/Users/user/project/node_modules/demo-pkg/index.js": `
+				import "demo-pkg2"
+				console.log('unused import')
+			`,
+			"/Users/user/project/node_modules/demo-pkg2/index.js": `
+				export const foo = 123
+				console.log('hello')
+			`,
+			"/Users/user/project/node_modules/demo-pkg2/package.json": `
+				{
+					"sideEffects": false
+				}
+			`,
+		},
+		entryPaths: []string{"/Users/user/project/src/entry.js"},
+		options: config.Options{
+			Mode:          config.ModeBundle,
+			AbsOutputFile: "/out.js",
+		},
+	})
+}
+
 func TestJSONLoaderRemoveUnused(t *testing.T) {
 	dce_suite.expectBundled(t, bundled{
 		files: map[string]string{

internal/bundler/snapshots/snapshots_dce.txt

@@ -310,6 +310,15 @@ console.log("hello");
 // Users/user/project/src/entry.js
 console.log(demo_pkg_exports);
 
+================================================================================
+TestPackageJsonSideEffectsFalseNoWarningInNodeModulesIssue999
+---------- /out.js ----------
+// Users/user/project/node_modules/demo-pkg/index.js
+console.log("unused import");
+
+// Users/user/project/src/entry.js
+console.log("used import");
+
 ================================================================================
 TestPackageJsonSideEffectsFalseRemoveBareImportCommonJS
 ---------- /out.js ----------