Skip to content

Commit

Permalink
add a warning/debug log message for #3867
Browse files Browse the repository at this point in the history
  • Loading branch information
evanw committed Aug 16, 2024
1 parent a15bb51 commit 38e22ed
Show file tree
Hide file tree
Showing 4 changed files with 165 additions and 0 deletions.
94 changes: 94 additions & 0 deletions internal/bundler_tests/bundler_packagejson_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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:
`,
})
}
8 changes: 8 additions & 0 deletions internal/bundler_tests/snapshots/snapshots_packagejson.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,14 @@ TestCommonJSVariableInESMTypeModule
// entry.js
module.exports = null;

================================================================================
TestPackageJsonBadExportsDefaultWarningIssue3867
---------- /out.js ----------

================================================================================
TestPackageJsonBadExportsImportAndRequireWarningIssue3867
---------- /out.js ----------

================================================================================
TestPackageJsonBadMain
---------- /Users/user/project/out.js ----------
Expand Down
1 change: 1 addition & 0 deletions internal/logger/msg_ids.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
62 changes: 62 additions & 0 deletions internal/resolver/package_json.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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,
Expand All @@ -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,
Expand Down

0 comments on commit 38e22ed

Please sign in to comment.