chore(build): rename configs, remove dead code, fix runtime bugs#1141
Closed
chore(build): rename configs, remove dead code, fix runtime bugs#1141
Conversation
Renames:
- esbuild.cli.build.mjs → esbuild.cli.mjs
- esbuild.index.config.mjs → esbuild.index.mjs
- esbuild.config.mjs → esbuild.build.mjs
- esbuild-shared.mjs → esbuild-utils.mjs
Dead code removed:
- esbuild-plugin-dead-code-elimination.mjs (unused plugin)
- scripts/esbuild.config.mjs (broken orphan, imported non-existent file)
- socketPackages={} + resolvePackageSubpath + resolve-socket-packages plugin (~50 lines never executed)
- template esbuild.build.mjs files (never called by any build script)
- INLINED_LEGACY_BUILD constant (no references in codebase)
- build:sea:internal:bootstrap script (referenced missing file)
- publish:sea script (referenced missing publish-sea.mjs)
- e2e:smol script (--smol not in BINARY_FLAGS, always errored)
- treeShaking:true (redundant default when bundle:true)
- external:[] (redundant default with platform:'node')
Refactored:
- createBuildRunner → runBuild (static imports, no IIFE, no dynamic imports)
- esbuild.build.mjs runs builds in-process via runBuild instead of spawning subprocesses
- Extracted resolveSocketLibExternal() helper (deduped 2x verbatim block)
- Extracted resolveConstant() inner fn (deduped 2x identical constants resolver)
- All template configs use runBuild() instead of duplicating manual build+write pattern
Runtime bugs fixed:
- test-wrapper.mjs: spawn() result used with .on() event listeners (Promise, not ChildProcess)
- validateBundle(): returned false|array mixed type; caller did violations.length on false
- verify-package.mjs: error(err) called undefined fn; logger used outside validate() scope;
fileExists() used forbidden fs.access(); unnecessary import process from node:process
- wasm.mjs: showHelp() used logger not in scope (only defined in error branch)
- patches.mjs: commitPatch() used logger not in scope; local logger shadowed module import
- build.mjs watch mode: watchResult.code accessed without null guard
- build-js.mjs: buildResult.code accessed without null guard
- cli-sentry template build.mjs: two spawn results accessed .code without null guard
- validate-bundle.mjs: .catch used console.error instead of logger
Consistency:
- All esbuild configs use sourcemap:false explicitly
- utf8 → utf-8 throughout
- All console.log/error → logger across .config/ and scripts/
- Stale JSDoc removed from cover.mjs helpers
- build-infra/package.json and cli/package.json indentation fixed
chore(build): Promise.allSettled, fix import order, update stale docs
- Switch Promise.all → Promise.allSettled in build.mjs phase 4 post-processing,
esbuild.build.mjs variant orchestration, and download-assets.mjs parallel
downloads so all tasks complete before reporting failures
- Fix import order in esbuild.build.mjs (const logger was between import groups)
- docs/build-guide.md: update config file names (esbuild.cli.mjs, esbuild.index.mjs,
esbuild.build.mjs) to match renames from previous commit
- build-infra/README.md: remove deleted deadCodeEliminationPlugin section and
stale usage examples; update Related Files to current config names
chore(build): Promise.allSettled in download-iocraft-binaries
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| const failed = results.filter(r => r.status === 'rejected') | ||
| if (failed.length > 0) { | ||
| process.exitCode = 1 | ||
| } |
There was a problem hiding this comment.
Orchestrator failure detection is unreachable dead code
Medium Severity
The runBuild function catches all errors internally and never rejects — it always resolves to undefined. The orchestrator uses Promise.allSettled and filters for r.status === 'rejected', which can therefore never match. The failed array is always empty and process.exitCode = 1 in the orchestrator is unreachable. The build still exits correctly because runBuild itself sets process.exitCode = 1, but the orchestrator's failure-detection logic is dead code and gives a false impression of where error handling occurs.
Additional Locations (1)
| const failed = results.filter(r => !r.ok) | ||
| const failed = settled.filter( | ||
| r => r.status === 'fulfilled' && !r.value.ok, | ||
| ) |
There was a problem hiding this comment.
Download error filter ignores rejected promises silently
Low Severity
The Promise.allSettled filter only checks r.status === 'fulfilled' && !r.value.ok, so any promise that actually rejects (e.g. due to an unexpected throw escaping the inner catch) would be silently ignored — no error logged, no process.exitCode set. The old Promise.all would have propagated such rejections. Adding a check for r.status === 'rejected' entries would close this gap.
Additional Locations (1)
Contributor
Author
|
Superseded by #1140 which already includes these changes. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
esbuild.cli.mjs,esbuild.index.mjs,esbuild.build.mjs,esbuild-utils.mjs)createBuildRunner→runBuild(static imports, no IIFE, no dynamic imports); extractsresolveSocketLibExternal()andresolveConstant()helpers to deduplicate repeated blocksspawn()result misused with.on(), mixed return type invalidateBundle(), undefined function refs, missing null guards, scoping issues, forbiddenfs.access()usagePromise.all→Promise.allSettledfor parallel build/download tasksutf8→utf-8,console.*→ logger, removes stale JSDocTest plan
pnpm run buildcompletes without errorspnpm run build --forcerebuilds cleanlyNote
Medium Risk
Medium risk because it rewires the CLI build orchestration/config entrypoints and touches multiple build/test scripts; failures would surface as broken builds or packaging rather than runtime user-facing logic.
Overview
Modernizes the CLI esbuild setup by renaming/splitting configs to
esbuild.cli.mjs,esbuild.index.mjs, and addingesbuild.build.mjsas the new orchestrator, then updates build scripts/templates/docs to call the new entrypoints.Removes dead build infrastructure (the
deadCodeEliminationPluginand several unused/orphaned esbuild scripts/configs) and refactors shared build helpers by replacingcreateBuildRunnerwith a singlerunBuild()implementation inesbuild-utils.mjs.Hardens build/test tooling by switching key parallel tasks to
Promise.allSettled, adding null/exit-code guards aroundspawn()results, normalizing file encodings toutf-8, and fixing error handling/return types in validation and packaging scripts.Written by Cursor Bugbot for commit 70074cf. Configure here.