Skip to content

Commit

Permalink
fix #3834: cli sometimes panics with --analyze
Browse files Browse the repository at this point in the history
  • Loading branch information
evanw committed Jul 25, 2024
1 parent 360d472 commit 892d2a7
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 15 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@
fs.open;
```

* Fix a panic when using the CLI with invalid build flags if `--analyze` is present ([#3834](https://github.com/evanw/esbuild/issues/3834))

Previously esbuild's CLI could crash if it was invoked with flags that aren't valid for a "build" API call and the `--analyze` flag is present. This was caused by esbuild's internals attempting to add a Go plugin (which is how `--analyze` is implemented) to a null build object. The panic has been fixed in this release.
## 0.23.0
**_This release deliberately contains backwards-incompatible changes._** To avoid automatically picking up releases like this, you should either be pinning the exact version of `esbuild` in your `package.json` file (recommended) or be using a version range syntax that only accepts patch upgrades such as `^0.22.0` or `~0.22.0`. See npm's documentation about [semver](https://docs.npmjs.com/cli/v6/using-npm/semver/) for more information.
Expand Down
40 changes: 25 additions & 15 deletions pkg/cli/cli_impl.go
Original file line number Diff line number Diff line change
Expand Up @@ -1023,11 +1023,15 @@ outer:
return
}

func isArgForBuild(arg string) bool {
return !strings.HasPrefix(arg, "-") || arg == "--bundle"
}

// This returns either BuildOptions, TransformOptions, or an error
func parseOptionsForRun(osArgs []string) (*api.BuildOptions, *api.TransformOptions, parseOptionsExtras, *cli_helpers.ErrorWithNote) {
// If there's an entry point or we're bundling, then we're building
for _, arg := range osArgs {
if !strings.HasPrefix(arg, "-") || arg == "--bundle" {
if isArgForBuild(arg) {
options := newBuildOptions()

// Apply defaults appropriate for the CLI
Expand Down Expand Up @@ -1091,20 +1095,25 @@ const (
)

func filterAnalyzeFlags(osArgs []string) ([]string, analyzeMode) {
analyze := analyzeDisabled
end := 0
for _, arg := range osArgs {
switch arg {
case "--analyze":
analyze = analyzeEnabled
case "--analyze=verbose":
analyze = analyzeVerbose
default:
osArgs[end] = arg
end++
if isArgForBuild(arg) {
analyze := analyzeDisabled
end := 0
for _, arg := range osArgs {
switch arg {
case "--analyze":
analyze = analyzeEnabled
case "--analyze=verbose":
analyze = analyzeVerbose
default:
osArgs[end] = arg
end++
}
}
return osArgs[:end], analyze
}
}
return osArgs[:end], analyze
return osArgs, analyzeDisabled
}

// Print metafile analysis after the build if it's enabled
Expand Down Expand Up @@ -1150,10 +1159,11 @@ func runImpl(osArgs []string, plugins []api.Plugin) int {
// Add any plugins from the caller after parsing the build options
if buildOptions != nil {
buildOptions.Plugins = append(buildOptions.Plugins, plugins...)
}

if analyze != analyzeDisabled {
addAnalyzePlugin(buildOptions, analyze, osArgs)
// The "--analyze" flag is implemented as a plugin
if analyze != analyzeDisabled {
addAnalyzePlugin(buildOptions, analyze, osArgs)
}
}

switch {
Expand Down
21 changes: 21 additions & 0 deletions scripts/end-to-end-tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -8446,6 +8446,27 @@ tests.push(
}),
)

// Tests for analyze
tests.push(
test(['in.js', '--analyze', '--outfile=node.js'], {
'in.js': `let x = 1 + 2`,
}, {
expectedStderr: `
node.js 15b 100.0%
└ in.js 15b 100.0%
`,
}),
test(['in.js', '--invalid-flag', '--analyze'], {
'in.js': `let x = 1 + 2`,
}, {
expectedStderr: `${errorIcon} [ERROR] Invalid build flag: "--invalid-flag"\n\n`,
}),
test(['--analyze'], {}, {
expectedStderr: `${errorIcon} [ERROR] Invalid transform flag: "--analyze"\n\n`,
}),
)

// Test writing to stdout
tests.push(
// These should succeed
Expand Down

0 comments on commit 892d2a7

Please sign in to comment.