add a warning/debug log message for #3867

evanw · Aug 16, 2024 · 38e22ed · 38e22ed
1 parent a15bb51
commit 38e22ed
Show file tree

Hide file tree

Showing 4 changed files with 165 additions and 0 deletions.
diff --git a/internal/bundler_tests/bundler_packagejson_test.go b/internal/bundler_tests/bundler_packagejson_test.go
@@ -2908,3 +2908,97 @@ func TestPackageJsonSubpathImportNodeBuiltinIssue3485(t *testing.T) {
 		},
 	})
 }
+
+// See: https://github.com/evanw/esbuild/issues/3867
+func TestPackageJsonBadExportsImportAndRequireWarningIssue3867(t *testing.T) {
+	packagejson_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/entry.js": `
+				import "foo"
+			`,
+			"/node_modules/foo/package.json": `
+				{
+					"exports": {
+						".": {
+							"import": "./dist/node/index.js",
+							"require": "./dist/node/index.cjs",
+							"node": {
+								"import": "./dist/node/index.js",
+								"require": "./dist/node/index.cjs"
+							},
+							"browser": {
+								"import": "./dist/browser/index.js",
+								"require": "./dist/browser/index.cjs"
+							},
+							"worker": {
+								"import": "./dist/browser/index.js",
+								"require": "./dist/browser/index.cjs"
+							}
+						}
+					}
+				}
+			`,
+			"/node_modules/foo/dist/node/index.js":     ``,
+			"/node_modules/foo/dist/node/index.cjs":    ``,
+			"/node_modules/foo/dist/browser/index.js":  ``,
+			"/node_modules/foo/dist/browser/index.cjs": ``,
+		},
+		entryPaths: []string{"/entry.js"},
+		options: config.Options{
+			Mode:          config.ModeBundle,
+			Platform:      config.PlatformNode,
+			AbsOutputFile: "/out.js",
+		},
+		debugLogs: true,
+		expectedScanLog: `node_modules/foo/package.json: DEBUG: The conditions "node" and "browser" and "worker" here will never be used as they come after both "import" and "require"
+node_modules/foo/package.json: NOTE: The "import" condition comes earlier and will be used for all "import" statements:
+node_modules/foo/package.json: NOTE: The "require" condition comes earlier and will be used for all "require" calls:
+`,
+	})
+}
+
+// See: https://github.com/evanw/esbuild/issues/3867
+func TestPackageJsonBadExportsDefaultWarningIssue3867(t *testing.T) {
+	packagejson_suite.expectBundled(t, bundled{
+		files: map[string]string{
+			"/entry.js": `
+				import "foo"
+			`,
+			"/node_modules/foo/package.json": `
+				{
+					"exports": {
+						".": {
+							"default": "./dist/node/index.js",
+							"node": {
+								"import": "./dist/node/index.js",
+								"require": "./dist/node/index.cjs"
+							},
+							"browser": {
+								"import": "./dist/browser/index.js",
+								"require": "./dist/browser/index.cjs"
+							},
+							"worker": {
+								"import": "./dist/browser/index.js",
+								"require": "./dist/browser/index.cjs"
+							}
+						}
+					}
+				}
+			`,
+			"/node_modules/foo/dist/node/index.js":     ``,
+			"/node_modules/foo/dist/node/index.cjs":    ``,
+			"/node_modules/foo/dist/browser/index.js":  ``,
+			"/node_modules/foo/dist/browser/index.cjs": ``,
+		},
+		entryPaths: []string{"/entry.js"},
+		options: config.Options{
+			Mode:          config.ModeBundle,
+			Platform:      config.PlatformNode,
+			AbsOutputFile: "/out.js",
+		},
+		debugLogs: true,
+		expectedScanLog: `node_modules/foo/package.json: DEBUG: The conditions "node" and "browser" and "worker" here will never be used as they come after "default"
+node_modules/foo/package.json: NOTE: The "default" condition comes earlier and will always be chosen:
+`,
+	})
+}
diff --git a/internal/bundler_tests/snapshots/snapshots_packagejson.txt b/internal/bundler_tests/snapshots/snapshots_packagejson.txt
@@ -3,6 +3,14 @@ TestCommonJSVariableInESMTypeModule
 // entry.js
 module.exports = null;
 
+================================================================================
+TestPackageJsonBadExportsDefaultWarningIssue3867
+---------- /out.js ----------
+
+================================================================================
+TestPackageJsonBadExportsImportAndRequireWarningIssue3867
+---------- /out.js ----------
+
 ================================================================================
 TestPackageJsonBadMain
 ---------- /Users/user/project/out.js ----------

diff --git a/internal/logger/msg_ids.go b/internal/logger/msg_ids.go
@@ -74,6 +74,7 @@ const (
 
 	// package.json
 	MsgID_PackageJSON_FIRST // Keep this first
+	MsgID_PackageJSON_DeadCondition
 	MsgID_PackageJSON_InvalidBrowser
 	MsgID_PackageJSON_InvalidImportsOrExports
 	MsgID_PackageJSON_InvalidSideEffects

diff --git a/internal/resolver/package_json.go b/internal/resolver/package_json.go
@@ -675,6 +675,16 @@ func parseImportsExportsMap(source logger.Source, log logger.Log, json js_ast.Ex
 			firstToken := logger.Range{Loc: expr.Loc, Len: 1}
 			isConditionalSugar := false
 
+			type DeadCondition struct {
+				reason string
+				ranges []logger.Range
+				notes  []logger.MsgData
+			}
+			var foundDefault logger.Range
+			var foundImport logger.Range
+			var foundRequire logger.Range
+			var deadCondition DeadCondition
+
 			for i, property := range e.Properties {
 				keyStr, _ := property.Key.Data.(*js_ast.EString)
 				key := helpers.UTF16ToString(keyStr.Value)
@@ -697,6 +707,34 @@ func parseImportsExportsMap(source logger.Source, log logger.Log, json js_ast.Ex
 					}
 				}
 
+				// Track "dead" conditional branches that can never be reached
+				if foundDefault.Len != 0 || (foundImport.Len != 0 && foundRequire.Len != 0) {
+					deadCondition.ranges = append(deadCondition.ranges, keyRange)
+					if deadCondition.reason == "" {
+						if foundDefault.Len != 0 {
+							deadCondition.reason = "\"default\""
+							deadCondition.notes = []logger.MsgData{
+								tracker.MsgData(foundDefault, "The \"default\" condition comes earlier and will always be chosen:"),
+							}
+						} else {
+							deadCondition.reason = "both \"import\" and \"require\""
+							deadCondition.notes = []logger.MsgData{
+								tracker.MsgData(foundImport, "The \"import\" condition comes earlier and will be used for all \"import\" statements:"),
+								tracker.MsgData(foundRequire, "The \"require\" condition comes earlier and will be used for all \"require\" calls:"),
+							}
+						}
+					}
+				} else {
+					switch key {
+					case "default":
+						foundDefault = keyRange
+					case "import":
+						foundImport = keyRange
+					case "require":
+						foundRequire = keyRange
+					}
+				}
+
 				entry := pjMapEntry{
 					key:      key,
 					keyRange: keyRange,
@@ -715,6 +753,30 @@ func parseImportsExportsMap(source logger.Source, log logger.Log, json js_ast.Ex
 			// PATTERN_KEY_COMPARE which orders in descending order of specificity.
 			sort.Stable(expansionKeys)
 
+			// Warn about "dead" conditional branches that can never be reached
+			if deadCondition.reason != "" {
+				kind := logger.Warning
+				if helpers.IsInsideNodeModules(source.KeyPath.Text) {
+					kind = logger.Debug
+				}
+				var conditions string
+				conditionWord := "condition"
+				itComesWord := "it comes"
+				if len(deadCondition.ranges) > 1 {
+					conditionWord = "conditions"
+					itComesWord = "they come"
+				}
+				for i, r := range deadCondition.ranges {
+					if i > 0 {
+						conditions += " and "
+					}
+					conditions += source.TextForRange(r)
+				}
+				log.AddIDWithNotes(logger.MsgID_PackageJSON_DeadCondition, kind, &tracker, deadCondition.ranges[0],
+					fmt.Sprintf("The %s %s here will never be used as %s after %s", conditionWord, conditions, itComesWord, deadCondition.reason),
+					deadCondition.notes)
+			}
+
 			return pjEntry{
 				kind:          pjObject,
 				firstToken:    firstToken,