Uh oh!
There was an error while loading. Please reload this page.
NP-790: Upgrade action to Node 24 LTS and modernize TypeScript toolchain - #2
Conversation
There was a problem hiding this comment.
Pull request overview
This PR upgrades the GitHub Action and its CI/tooling to the Node.js 24 runtime, modernizes the TypeScript/Jest/ESLint toolchain, and adapts code/tests to newer major versions (notably @actions/github v6 / Octokit REST API changes).
Changes:
- Migrate action runtime to
node24and update CI workflow to usesetup-node@v6+ addnpm run lint. - Upgrade major dependencies (TypeScript 5.9, Jest 30, Prettier 3, ESLint 9, semantic-release packages) and add new TS configs for lint/tests.
- Refactor source to use
octokit.rest.*, remove customAwait<T>in favor ofAwaited<T>, and add Jest stubs for ESM-only semantic-release modules.
Reviewed changes
Copilot reviewed 25 out of 35 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
action.yml | Switch action runtime to node24 and refresh metadata formatting. |
package.json | Enforce Node 24 engine, add lint/format scripts, upgrade deps/devDeps. |
.github/workflows/test.yml | Update checkout/setup-node actions, add lint step, run build/test/check. |
tsconfig.json / tsconfig.test.json / tsconfig.eslint.json | Modernize TS target/options and introduce separate configs for Jest + ESLint type-aware linting. |
eslint.config.mjs | Add flat ESLint config with type-checked rules and test relaxations. |
src/github.ts | Update Octokit usage to octokit.rest.* APIs and tighten types. |
src/action.ts / src/utils.ts / src/main.ts | Improve typing, error handling, and release-type validation; adjust custom rules mapping and logging. |
types/semantic.d.ts | Add/strengthen ambient types for ESM-only semantic-release packages. |
tests/* + tests/__mocks__/* | Update tests for new Octokit shape/Jest APIs; add ESM stubs. |
lib/* | Check in updated build output targeting Node 24. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Stringify via JSON so we don't fall back to "[object Object]" for | ||
| // plain-object causes; fall back to String() for non-serialisable | ||
| // values (e.g. BigInt, circular refs). | ||
| let serialised: string; | ||
| try { | ||
| serialised = JSON.stringify(error.cause); | ||
| } catch { | ||
| serialised = Object.prototype.toString.call(error.cause); | ||
| } |
There was a problem hiding this comment.
The fallback when JSON.stringify(error.cause) throws uses Object.prototype.toString.call(error.cause), which loses useful details (e.g. 10n becomes "[object BigInt]", plain objects become "[object Object]") and contradicts the preceding comment about falling back to String(). Consider using String(error.cause) for primitives and util.inspect(error.cause, { depth: ..., breakLength: ... }) (handles circular refs) for objects so the logged cause remains actionable.
| return customReleaseTypes | ||
| .split(releaseRuleSeparator) | ||
| .filter((customReleaseRule) => { | ||
| const parts = customReleaseRule.split(releaseTypeSeparator); | ||
| const rawType = parts[0]; | ||
| const rawRelease = parts[1]; | ||
| if (parts.length < 2) { | ||
| if (rawType === undefined || rawRelease === undefined) { | ||
| core.warning( | ||
| `${customReleaseRule} is not a valid custom release definition.` | ||
| ); | ||
| return false; | ||
| } | ||
| const defaultRule = defaultChangelogRules[parts[0].toLowerCase()]; | ||
| if (customReleaseRule.length !== 3) { | ||
| const defaultRule = defaultChangelogRules[rawType.toLowerCase()]; | ||
| if (parts.length !== 3) { | ||
| core.debug( | ||
| `${customReleaseRule} doesn't mention the section for the changelog.` | ||
| ); | ||
| core.debug( | ||
| defaultRule | ||
| ? `Default section (${defaultRule.section}) will be used instead.` | ||
| ? `Default section (${defaultRule.section ?? ''}) will be used instead.` | ||
| : "The commits matching this rule won't be included in the changelog." | ||
| ); | ||
| } | ||
| if (!DEFAULT_RELEASE_TYPES.includes(parts[1])) { | ||
| core.warning(`${parts[1]} is not a valid release type.`); | ||
| if (!DEFAULT_RELEASE_TYPES.includes(rawRelease)) { | ||
| core.warning(`${rawRelease} is not a valid release type.`); | ||
| return false; |
There was a problem hiding this comment.
mapCustomReleaseRules splits on , and : but never trims whitespace. Inputs like hotfix:patch, pre-feat:preminor (note the space after the comma) will produce a rawType with a leading space and a rawRelease that may fail validation, even though the user input is reasonable. Consider trimming each rule and each part (and possibly filtering out empty rules) before validating/mapping.
| description: 'Which type of bump to use when none explicitly provided when commiting to a release branch (default: `patch`).' | ||
| required: false | ||
| default: "patch" | ||
| default: 'patch' | ||
| default_prerelease_bump: | ||
| description: "Which type of bump to use when none explicitly provided when commiting to a prerelease branch (default: `prerelease`)." | ||
| description: 'Which type of bump to use when none explicitly provided when commiting to a prerelease branch (default: `prerelease`).' |
There was a problem hiding this comment.
Typo in input descriptions: "commiting" should be "committing" (appears in both the release-branch and prerelease-branch descriptions).
0815258 to
9d6c132CompareRuntime and CI: - action.yml now uses runs.using: node24 (was node16) - Add .nvmrc pinning Node 24 and engines.node >= 24.0.0 in package.json - Project is native ESM: package.json "type": "module", tsconfig.json module/moduleResolution: NodeNext, so every relative import in src/**/*.ts and tests/**/*.ts carries a .js extension per spec - Compiled lib/*.js is native ESM; GitHub's node24 runtime loads it directly via Node's own ESM loader - Add a project-level .npmrc pinning registry=https://registry.npmjs.org/ so CI does not inherit a developer's private-registry config - .github/workflows/test.yml bumps actions/checkout and actions/setup-node to v6 and runs lint + check + test + build Dependency upgrades (aggressive majors, zero known vulnerabilities): - @actions/core 1.x -> 3.0.1 (pure ESM) - @actions/exec 1.x -> 3.0.0 (pure ESM) - @actions/github 4.x -> 9.1.1 (fixes the broken @octokit/core/dist-types/types deep import from 8.0.1 + reaches @octokit/core@7, plugin-paginate-rest@14, plugin-rest-endpoint-methods@17, request@10, request-error@7, undici@6) - @semantic-release/commit-analyzer 8 -> 13 (pure ESM) - @semantic-release/release-notes-generator 9 -> 14 (pure ESM) - conventional-changelog-conventionalcommits 4 -> 9 (pure ESM) - typescript 4.4 -> 5.9, @types/node 16 -> 24, prettier 2 -> 3 - New: eslint 9 (flat config), typescript-eslint 8 (strict + stylistic), @eslint/js, globals Source changes: - src/github.ts: migrated to octokit.rest.{repos,git}.* - Deleted src/ts.ts; replaced the custom Await<T> helper with the built-in TypeScript Awaited<T> - src/action.ts: added VALID_RELEASE_TYPES set + isReleaseType type guard replacing the prior `as ReleaseType` cast; release_type output is only set after inc() succeeds - src/main.ts: catch (error: unknown) with narrowing that surfaces error.stack and recursively logs error.cause - src/utils.ts: dropped the @ts-ignore on default-release-types by inlining the constant list (the internal module is not a public export), added explicit MappedReleaseRule return type, refactored for noUncheckedIndexedAccess - types/semantic.d.ts: tightened `any` -> structural shapes Tooling: - tsconfig.json: target ES2023, strict + noUncheckedIndexedAccess + useUnknownInCatchVariables, module/moduleResolution NodeNext - tsconfig.eslint.json drives type-aware ESLint across src/**, tests/**, types/**, *.mjs, and vitest.config.ts - eslint.config.mjs (flat config) with strictTypeChecked + stylisticTypeChecked, plus a relaxed override for tests/**/*.ts that still enforces no-unused-vars with the ^_ escape hatch - Prettier 3 is happy across the tree; .prettierignore keeps LICENSE, .nvmrc, and binary assets out of the formatter Testing (Vitest 4): - Replaced Jest 30 + ts-jest + @types/jest + @jest/globals with vitest@^4.1.5 as the single dev dependency for the test stack; dropped jest.config.mjs, tsconfig.test.json, and the local tests/__mocks__/@semantic-release/* stubs entirely because Vitest loads the real ESM packages natively - Scripts: \"test\": \"vitest run\", \"test:watch\": \"vitest\" - New vitest.config.ts is ~20 lines: node environment, clearMocks + restoreMocks, 10s timeout, no globals - tests/github.test.ts: vi.mock('@actions/github', factory) hoisted above static imports; no more dynamic imports or awaited factories - tests/action.test.ts + tests/utils.test.ts: * vi.mock('@actions/core', async importOriginal => { ...actual, debug: vi.fn(), info: vi.fn(), setOutput: vi.fn(), setFailed: vi.fn() }) because Node seals the real ESM namespace and vi.spyOn cannot mutate it in place * vi.mock('../src/utils.js') and vi.mock('../src/github.js') with the same pattern; per-test behaviour goes through vi.mocked(utils.getCommits).mockImplementation(...) because ESM named imports are live bindings and spyOn on a namespace cannot redirect them * tests/action.test.ts installs the console.info suppression in beforeEach (restoreMocks: true restores any module-scope spy after the first test completes) - Built-in Node imports use the node: specifier (node:fs, node:path) Build output: - .gitignore no longer excludes lib/; the checked-in build is native ESM targeting Node 24 Validation: - 34/34 tests pass under Vitest (~0.6s) - npm run lint clean (ESLint strict + stylistic) - npm run check clean (Prettier 3) - npm run build clean (tsc -> native ESM in lib/) - npm audit: 0 vulnerabilities Made-with: Cursor
9d6c132 to
5d1d8dbCompareThere was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 36 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
README.md:43
- The README usage example still references
mathieudutour/github-tag-action@v6.1, but this PR bumps the package major version to 7.0.0. Update the example to point at the new major (e.g.@v7or the exact@v7.0.0) so users don't copy/paste an outdated version.
- uses: actions/checkout@v6
- name: Bump version and push tag
id: tag_version
uses: mathieudutour/github-tag-action@v6.1
with:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "@actions/core": "^3.0.1", | ||
| "@actions/exec": "^3.0.0", | ||
| "@actions/github": "^9.1.1", | ||
| "@semantic-release/commit-analyzer": "^13.0.1", |
There was a problem hiding this comment.
PR description says @actions/github is upgraded from v4 to v6, but package.json sets it to ^9.1.1. Please update the PR description (or the dependency version) so the documented upgrade matches what will actually be shipped.
Uh oh!
There was an error while loading. Please reload this page.
Runtime and CI:
module/moduleResolution: NodeNext, so every relative import in
src//*.ts and tests//*.ts carries a .js extension per spec
directly via Node's own ESM loader
so CI does not inherit a developer's private-registry config
to v6 and runs lint + check + test + build
Dependency upgrades (aggressive majors, zero known vulnerabilities):
deep import from 8.0.1 + reaches @octokit/core@7,
plugin-paginate-rest@14, plugin-rest-endpoint-methods@17,
request@10, request-error@7, undici@6)
@eslint/js, globals
Source changes:
built-in TypeScript Awaited
guard replacing the prior
as ReleaseTypecast; release_type outputis only set after inc() succeeds
error.stack and recursively logs error.cause
inlining the constant list (the internal module is not a public
export), added explicit MappedReleaseRule return type, refactored
for noUncheckedIndexedAccess
any-> structural shapesTooling:
useUnknownInCatchVariables, module/moduleResolution NodeNext
src/, tests/, types/**, *.mjs, and vitest.config.ts
stylisticTypeChecked, plus a relaxed override for tests/**/*.ts that
still enforces no-unused-vars with the ^_ escape hatch
.nvmrc, and binary assets out of the formatter
Testing (Vitest 4):
vitest@^4.1.5 as the single dev dependency for the test stack;
dropped jest.config.mjs, tsconfig.test.json, and the local
tests/mocks/@semantic-release/* stubs entirely because Vitest
loads the real ESM packages natively
restoreMocks, 10s timeout, no globals
above static imports; no more dynamic imports or awaited factories
debug: vi.fn(), info: vi.fn(), setOutput: vi.fn(), setFailed: vi.fn() })
because Node seals the real ESM namespace and vi.spyOn cannot
mutate it in place
the same pattern; per-test behaviour goes through
vi.mocked(utils.getCommits).mockImplementation(...) because ESM
named imports are live bindings and spyOn on a namespace cannot
redirect them
beforeEach (restoreMocks: true restores any module-scope spy
after the first test completes)
Build output:
ESM targeting Node 24
Validation: