Conversation
18f44d3 to
b5324b8
Compare
dscho
left a comment
There was a problem hiding this comment.
@Shadowghost thank you for working on this! Quite a bit of work you had to do there before the PR build passed, thanks for seeing it through!
I am a bit concerned, though: While I agree that it would be good to move from Node.JS v20 to v24, I am not on board switching away from ncc and Jest without good reason. And I don't see any good reason.
package.json
Outdated
| "format-check": "prettier --check **/*.ts", | ||
| "lint": "eslint **/*.ts", | ||
| "package": "ncc build --source-map", | ||
| "package": "esbuild main.ts --bundle --platform=node --target=node24 --format=esm --sourcemap --outfile=dist/index.js", |
There was a problem hiding this comment.
Hmm. I have no experience whatsoever with esbuild, but tons with ncc. And I work in other projects that seem not to have any problems with ncc building ECMAScript Modules... (see e.g. here).
Changing the bundling system to something completely different was more than I bargained for when I started reviewing a PR to upgrade Node.JS v20 to v24...
| @@ -1,4 +1,5 @@ | |||
| /* eslint no-console: "off" */ | |||
| import {test, expect} from 'vitest' | |||
There was a problem hiding this comment.
I have tons of experience with Jest, but none with vitest.
Changing the test framework as part of a PR to upgrade the minimum Node.JS version from v20 to v24 was not quite what I had in mind...
|
Did a bit more testing:
The main reason why this is an issue are the upgraded |
package.json
Outdated
| "format-check": "prettier --check **/*.ts", | ||
| "lint": "eslint **/*.ts", | ||
| "package": "esbuild main.ts --bundle --platform=node --target=node24 --format=esm --sourcemap --outfile=dist/index.js", | ||
| "package": "esbuild main.ts --bundle --platform=node --target=node24 --format=esm --sourcemap --outfile=dist/index.js --banner:js=\"import { createRequire } from 'module'; const require = createRequire(import.meta.url);\"", |
There was a problem hiding this comment.
This is in line with what Claude Opus warns me about when switching from ncc to esbuild:
- ncc is the conservative, purpose‑built choice for JS/TS GitHub Actions.
- esbuild can work, but only if you are disciplined about CJS output, externals, and runtime testing of the bundled artifact.
- The most common breakages when switching are dynamic
require(), optional deps, and CJS/ESM interop — exactly the things Actions hit.
If the motivation is speed, esbuild is attractive. If the motivation is reliability, ncc still wins for Actions.
Why GitHub Actions are special
GitHub Actions (Node 20/22 runtime) have these properties:
- Node-only execution (no browser assumptions)
- Single entrypoint (
dist/index.js) - Heavy use of:
require()(directly or indirectly via deps)- optional dependencies
- conditional imports based on platform / Node version
- You ship one JS file into the repo (no
node_modules)
This is exactly the problem ncc was designed for:
“Compile a Node.js project into a single file. Supports TypeScript, binary addons, dynamic requires.” [github.com], [npmjs.com]
esbuild is more general‑purpose and faster, but it does not optimize for these Node edge cases.
The big gotchas when replacing ncc with esbuild (for Actions)
1. You must emit CommonJS
For GitHub Actions, CJS is the safe choice.
Real‑world guidance from Node+esbuild users:
When bundling for
--platform=nodeand mixing CJS/ESM deps,--format=cjsworks in most cases;--format=esmfrequently triggers runtime failures. [dev.to]
If someone proposes:
esbuild --platform=node --format=esmthat’s a red flag for Actions unless they can prove runtime parity.
What to look for in the PR
--platform=node--format=cjs- No
"type": "module"surprises
2. Dynamic require() is the #1 Action killer
This is the classic failure mode:
“Dynamic require of '' is not supported”
occurs when arequire()survives into the bundle and esbuild can’t statically resolve it. [github.com], [stackoverflow.com]
Why Actions hit this often:
@actions/*packages- optional deps (e.g.
fsevents, platform‑specific stuff) - libraries that do
require(someVariable)
ncc explicitly advertises handling this class of problem. esbuild does not guarantee it.
Required mitigation if using esbuild
- Keep output CJS
- Be very explicit about
externalvs bundled deps - Test the bundled file under
node dist/index.js, not just unit tests
3. Native addons / non-JS assets
ncc explicitly claims support for binary addons.
esbuild does not magically solve this. [github.com]
If your Action (or its deps) touch:
.nodebinaries- wasm
- JSON schemas loaded via
fs - templates / certificates / text assets
then esbuild requires manual handling (copy steps, externals, loaders).
Many “it works locally” Action failures come from missing runtime files.
4. “Single file” still means runtime behavior must match
GitHub Actions don’t care how you bundle — only that runs.main works.
Common esbuild migration mistake:
- ✅ build succeeds
- ✅ tests pass against TS sources
- ❌ Action fails at runtime because:
- optional dep path changed
__dirnameassumptions broke- dynamic require wasn’t traced
ncc users hit this less often because the tool is Node‑deployment‑biased by design.
Is esbuild “safe” for Actions?
From a security / maintenance standpoint:
- No direct vulnerabilities found in Snyk’s database for
esbuild(latest versions). [security.snyk.io] - esbuild is very actively released (frequent version updates). [security.snyk.io]
- It ships platform‑specific binaries (e.g.
@esbuild/win32-x64), which is normal but sometimes triggers supply‑chain scrutiny in orgs. [socket.dev]
So “safe” in the OSS sense: yes.
“Safe” in the GitHub Action runtime‑parity sense: only if configured carefully.
Maintenance comparison (relevant to Actions)
- esbuild: extremely active, fast releases, core infrastructure tool. [security.snyk.io]
- ncc: still maintained, but slower cadence; narrower scope; very stable API. [github.com], [github.com]
For Actions, stability > features is usually the right trade.
My recommendation (Action‑specific)
Stick with ncc if:
- The Action is customer‑facing or security‑sensitive
- You rely on optional deps or Node ecosystem quirks
- The current build is boring and works
Consider esbuild if:
- Build time matters (monorepo, many Actions)
- You are willing to:
- lock output to CJS
- add runtime smoke tests on the bundled file
- own the occasional Node edge case
Absolute minimum bar for an esbuild PR
I’d insist on:
--platform=node --format=cjs- A CI step that runs the bundled output (
node dist/index.js) - Explicit justification for any
externaldeps - A rollback plan (ncc config kept around)
There was a problem hiding this comment.
The way I read this is: Switching to esbuild would buy me a lot of trouble.
|
@dscho reverted back to ncc |
Since GitHub will force Node 24 on June 2nd, 2026:
ESlint needs a bump once the GitHub plugin supports v10