From 38e22ed7a73b245c9c175713f96f734274b17263 Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Fri, 16 Aug 2024 16:30:19 -0400 Subject: [PATCH] add a warning/debug log message for #3867 --- .../bundler_tests/bundler_packagejson_test.go | 94 +++++++++++++++++++ .../snapshots/snapshots_packagejson.txt | 8 ++ internal/logger/msg_ids.go | 1 + internal/resolver/package_json.go | 62 ++++++++++++ 4 files changed, 165 insertions(+) diff --git a/internal/bundler_tests/bundler_packagejson_test.go b/internal/bundler_tests/bundler_packagejson_test.go index 8843f82d58c..19aa3ee99ca 100644 --- 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 index eaf8239cb51..16a55913d12 100644 --- 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 index bcc9c9b2ce9..2e1e305ca4b 100644 --- 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 index 409349bbe3b..acf5a220f60 100644 --- 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,