Adopt TypeScript under Node 24 #734
Conversation
package.json
Outdated
| "devDependencies": { | ||
| "@tsconfig/node-ts": "^23.6.1", | ||
| "@tsconfig/node24": "^24.0.1", | ||
| "@types/node": "^24.2.1", |
There was a problem hiding this comment.
We don't want Dependabot to bump the major version here so may need to figure out how best to do that.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
Code wise this is ready but need to:
This did catch one edge case bug in |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
@Bo98 this looks awesome, do you need any help on the remaining items? If so, the first two make sense to me but I may need a pointer on how to do 3 |
There was a problem hiding this comment.
Pull request overview
This PR migrates GitHub Actions from Node 20 to Node 24, leveraging Node 24's native TypeScript support to eliminate the need for a compilation step. The migration involves converting JavaScript files to TypeScript (.mts), adding type annotations, and updating action configurations.
Key changes:
- Converted all action files from
.mjsto.mtswith TypeScript type annotations - Updated all action.yml files to use
node24runtime instead ofnode20 - Added type safety improvements including explicit type annotations, non-null assertions, and error handling guards
Reviewed changes
Copilot reviewed 26 out of 518 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| limit-pull-requests/main.test.mts | Added type annotation for directory variable |
| limit-pull-requests/main.mts | Added type conversions for numeric inputs, non-null assertions for pull request data, and error type guard |
| limit-pull-requests/action.yml | Updated runtime from node20 to node24, changed entry point to .mts |
| label-pull-requests/main.test.mts | Added null check guard for GITHUB_EVENT_PATH before directory cleanup |
| label-pull-requests/main.mts | Added comprehensive type definitions, assertions, and replaced string matching with RegExp |
| label-pull-requests/action.yml | Updated runtime from node20 to node24, changed entry point to .mts |
| git-user-config/main.mts | Converted from CommonJS to ES modules, added error type guard, changed to top-level await |
| git-user-config/action.yml | Updated runtime from node20 to node24, changed entry point to .mts |
| git-try-push/main.mts | Converted from CommonJS to ES modules, added type conversion for numeric input, added error type guard, changed to top-level await |
| git-try-push/action.yml | Updated runtime from node20 to node24, changed entry point to .mts |
| find-related-workflow-run-id/main.mts | Added GraphQL response typing and optional chaining for nested properties |
| find-related-workflow-run-id/action.yml | Updated runtime from node20 to node24, changed entry point to .mts |
| dismiss-approvals/main.mts | Added parseInt for PR number input and error type guard |
| dismiss-approvals/action.yml | Updated runtime from node20 to node24, changed entry point to .mts |
| create-pull-request/main.mts | Added explicit type annotation for PR request parameters and error type guard |
| create-pull-request/action.yml | Updated runtime from node20 to node24, changed entry point to .mts |
| create-or-update-issue/main.mts | Added type annotation for listForRepo parameters and error type guard |
| create-or-update-issue/action.yml | Updated runtime from node20 to node24, changed entry point to .mts |
| check-commit-format/main.test.mts | Added null check guard for GITHUB_EVENT_PATH and new test cases for merge/empty commits |
| check-commit-format/main.mts | Added type annotations, empty commit detection, and improved array access |
| check-commit-format/action.yml | Updated runtime from node20 to node24, changed entry point to .mts |
| api-commit-and-push/main.test.mts | Added type annotations for test variables |
| api-commit-and-push/main.mts | Added type annotations, improved error handling with type guard, removed redundant file read |
| api-commit-and-push/action.yml | Updated runtime from node20 to node24, changed entry point to .mts |
| .github/workflows/vendor-node-modules.yml | Changed npm install to npm ci --omit=dev for more reliable builds |
| .github/workflows/test.yml | Updated Node version to 24 and added TypeScript type checking step |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (body) { | ||
| constraintApplies = body.match(constraint.pr_body_content) | ||
| constraintApplies = RegExp(constraint.pr_body_content).test(body) |
There was a problem hiding this comment.
Creating a new RegExp object on every iteration is inefficient. Consider compiling the regex once outside the loop or caching it, especially if this constraint is checked multiple times.
There was a problem hiding this comment.
constraint is the loop item so you can't just lift it out. And given it is only used once then caching doesn't make any sense.
| if (error.code !== "ENOENT") throw error; | ||
|
|
||
| content = null; | ||
| if (error instanceof Error && "code" in error && error.code === "ENOENT") { |
There was a problem hiding this comment.
The type guard for checking 'code' property is verbose. Consider using a type predicate function like 'isNodeError' or casting to 'NodeJS.ErrnoException' which has a typed 'code' property.
There was a problem hiding this comment.
Type casting is a bit dubious as it might not actually be a ErrnoException whereas "code" in error actually checks it. It doesn't really matter though given JavaScript at runtime allows basically anything anyway so this is all an argument about which semantics one prefers.
For point 3 what you'd do is open a test PR to homebrew-core that modifies Portable Ruby and also updates the test workflow to point to Because of the prebuild action stuff, you might need to open a test PR here first to repoint setup-homebrew and then repoint the prebuild action in homebrew-core test PR to the actions test PR. It's a bit messy but probably worthwhile. |
|
test-bot failure seems new and may need fixing in Homebrew/brew |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 511 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (const file of files.data) { | ||
| for (const file of files) { | ||
| // File object could have this field set as 'null' | ||
| if (!file.sha) { |
There was a problem hiding this comment.
The comment states that the sha field could be null, but when null the file.content field is not set before continuing. This creates inconsistency with other code paths where file.content is always defined. Consider setting file.content to an empty string or undefined for consistency.
| if (!file.sha) { | |
| if (!file.sha) { | |
| file.content = ""; |
There was a problem hiding this comment.
content is marked as optional above
| owner: owner, | ||
| repo: repo, | ||
| content: await fs.readFile(absoluteFile, {encoding: "base64"}), | ||
| content: content, |
There was a problem hiding this comment.
The variable 'content' is used here but was already read earlier at line 33. This redundant read was removed, but the variable is still being passed. Consider clarifying whether this change was intentional or if there's a logic issue.
There was a problem hiding this comment.
Maybe I'm reading this wrong but this comment doesn't make any sense?
@Bo98 correct and now fixed and 🟢 after rerun |
branchvincent
force-pushed
the
ts-node24
branch
from
5fbf58e to
e702ec5
Compare
|
Looks like it needs macOS 12, which actually weirdly enough means the old 10.11-cross image would have worked. Anyhow I'll get a 10.15-cross set up at the same time as the Xcode 26.2 updates (when that's available). There's also no rush here. I could probably split out the setup-homebrew changes to a separate PR. GHES doesn't even support Node 24 yet - the whole process is a bit less rushed this time so Node 20 won't be deprecated for a while longer. |
GitHub Actions now supports Node 24. Node 24 supports natively running TypeScript without needing a compile stage (erasable syntax only) which means we can adopt it without needing to add any build stages.
This pull request experiments with adopting TypeScript (for one action in this initial draft).
Given the Actions runner with support for Node 24 is just 2 weeks old, perhaps we want to allow a little more time before doing this since we don't do tagged releases?