chore: migrate from ncc (CJS) to rollup (ESM)#436
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Greptile SummaryMigrates the build toolchain from
Confidence Score: 5/5Safe to merge — the ESM migration is technically correct and all remaining findings are P2. All changed files look correct: the rollup config, tsconfig NodeNext settings, and the generated ESM bundle are consistent with the official actions/typescript-action template pattern. The only finding is a P2 discrepancy between the PR description (@actions/cache v6) and package.json (^4.0.0), which does not affect runtime behavior. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["src/index.ts"] -->|"rollup + plugin-typescript"| B["TypeScript transpilation"]
B -->|"plugin-commonjs ignoreTryCatch:false"| C["CJS dependency interop"]
C -->|"plugin-node-resolve preferBuiltins:true"| D["Node built-in resolution"]
D -->|"plugin-json"| E["JSON imports resolved"]
E -->|"plugin-license"| F["dist/licenses.txt"]
E --> G["dist/index.js ESM bundle"]
G --> H{"Root package.json type:module"}
H -->|"Yes"| I["Node.js ESM mode"]
I --> J["Action executes via node24"]
Reviews (8): Last reviewed commit: "[autofix.ci] apply automated fixes" | Re-trigger Greptile |
There was a problem hiding this comment.
Code Review
This pull request migrates the project's build system from @vercel/ncc to rollup to improve build configuration and dependency management. The changes include updating package.json dependencies, adding a rollup.config.ts file, and adjusting tsconfig.json settings. I have provided feedback regarding the redundancy of npx in npm scripts, the recommendation to explicitly mark Node.js built-ins as external in the Rollup configuration, and the need to restore license attribution previously handled by ncc.
| "format:write": "prettier --write **/*.ts", | ||
| "lint": "npx eslint . && npm run format:check", | ||
| "package": "ncc build -s src/index.ts --license licenses.txt", | ||
| "package": "npx rimraf ./dist && npx rollup --config rollup.config.ts --configPlugin @rollup/plugin-typescript", |
There was a problem hiding this comment.
The use of npx inside npm scripts is redundant because npm run automatically adds ./node_modules/.bin to the PATH during execution. Removing it simplifies the command and slightly improves performance.
| "package": "npx rimraf ./dist && npx rollup --config rollup.config.ts --configPlugin @rollup/plugin-typescript", | |
| "package": "rimraf dist && rollup --config rollup.config.ts --configPlugin @rollup/plugin-typescript", |
| import nodeResolve from '@rollup/plugin-node-resolve' | ||
| import typescript from '@rollup/plugin-typescript' | ||
|
|
||
| const config = { |
There was a problem hiding this comment.
For a Node.js bundle, it is recommended to explicitly mark Node.js built-in modules (like node:crypto, node:fs, etc.) as external. This prevents Rollup from attempting to resolve them or producing warnings about unresolved dependencies during the build process.
const config = {
external: [/node:/],
input: 'src/index.ts',| plugins: [ | ||
| typescript(), | ||
| nodeResolve({ preferBuiltins: true }), | ||
| commonjs(), | ||
| json() | ||
| ] |
There was a problem hiding this comment.
The migration from @vercel/ncc to rollup removes the automatic generation of a licenses.txt file (which was previously included via the --license flag). For GitHub Actions that bundle third-party dependencies, it is a best practice (and often a legal requirement for MIT/Apache licenses) to include attribution for those dependencies. Consider adding a plugin like rollup-plugin-license to regenerate this artifact.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f2de96e. Configure here.
Switch bundler from @vercel/ncc to rollup to support ESM-only @actions/toolkit v3 packages. This upgrades all @actions/* dependencies to their latest major versions and adds "type": "module" to package.json. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
minimatch 3.x uses `try { return require('path') } catch (e) {}` to
get the path module, falling back to `{ sep: '/' }`. With rollup's
commonjs plugin, `ignoreTryCatch: true` (the default) leaves
require('path') unconverted inside try blocks. At runtime in the ESM
bundle, require() is unavailable, so the catch fires and minimatch
uses '/' as the path separator — breaking all glob matching on Windows
where paths use backslashes.
Setting ignoreTryCatch: false ensures require('path') is converted to
an ESM import, giving minimatch the correct path.sep ('\\' on Windows).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add rollup-plugin-license to generate dist/licenses.txt for bundled third-party dependencies (replaces ncc's --license flag) - Remove redundant npx from npm scripts since npm run already adds node_modules/.bin to PATH Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Summary
@vercel/ncctorollupwith@rollup/plugin-commonjs,@rollup/plugin-node-resolve,@rollup/plugin-json, and@rollup/plugin-typescript"type": "module"topackage.jsonfor ESM support@actions/*dependencies to their latest major versions (@actions/corev3,@actions/execv3,@actions/cachev6,@actions/globv0.6,@actions/iov3)dist/licenses.txt,dist/sourcemap-register.js)Why
The
@actions/toolkitpackages v3+ are ESM-only and can't be bundled by ncc (which uses webpack with CJSrequire()). This is what's blocking #435 (renovate@actions/execv3 upgrade). The officialactions/typescript-actiontemplate has already migrated to rollup.Test plan
npm run all— format, lint, package)check-distworkflow passes (dist/index.js matches build output)🤖 Generated with Claude Code
Note
Medium Risk
Moderate risk because it changes the action’s build/bundling pipeline and module format (CJS→ESM), which can break runtime execution or dependency resolution if the generated
dist/output differs across environments.Overview
Migrates the GitHub Action build from
@vercel/ncc(CommonJS) to a Rollup-based ESM bundle, addingrollup.config.tsand updating TypeScript settings toNodeNextto support ESM output.Updates
package.jsonto"type": "module", switches the packaging script to Rollup, and upgrades@actions/*dependencies to their latest major (ESM-only) versions. The checked-indist/artifacts are regenerated accordingly (including license output) and legacy ncc-specific artifacts are removed.Reviewed by Cursor Bugbot for commit 59e728e. Bugbot is set up for automated code reviews on this repo. Configure here.