Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix update-node-version script for non-existent @types/node version #3788

Merged
merged 3 commits into from
Nov 7, 2024

Conversation

koesie10
Copy link
Member

@koesie10 koesie10 commented Nov 4, 2024

The Update Node version workflow is currently failing because a 20.18 version of @types/node does not exist even though VS Code is now using Node 20.18.0. This doesn't really matter for us since we can still use the 20.17 version of @types/node, but the script isn't checking for that.

This adds a check to the script that the version of @types/node we are requesting actually exists. If it doesn't, it will decrement the minor version (e.g. 20.18.0 becomes 20.17.0) and try that version instead. It will use the first existing version for the @types/node package (e.g. 20.17.* in this case).

@koesie10 koesie10 requested a review from a team November 4, 2024 14:40
@koesie10 koesie10 requested a review from a team as a code owner November 4, 2024 14:40
Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks right to me. Do you have an example of this running somewhere?

@koesie10
Copy link
Member Author

koesie10 commented Nov 5, 2024

This looks right to me. Do you have an example of this running somewhere?

I just ran the workflow manually which is available here. It created this PR.

Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Looks good.

This issue came about because we want to bump node to v20.18.0, but the latest typings is for v20.17.0. It's not a blocker or anything, but how will we know to upgrade the typings to v20.18.0 when that version finally comes out?

Perhaps there could be a workflow that runs on a schedule and attempts to reconcile any mismatch between the node version and the node typings. Or perhaps being off by a few minor versions is not a big enough deal to worry about.

@koesie10
Copy link
Member Author

koesie10 commented Nov 6, 2024

Perhaps there could be a workflow that runs on a schedule and attempts to reconcile any mismatch between the node version and the node typings. Or perhaps being off by a few minor versions is not a big enough deal to worry about.

Good point. I've changed this script to always make changes to all fields where the Node version is used (.nvmrc, engines.node, and devDependencies[@types/node]), regardless of what .nvmrc says. This should ensure that when a new version of @types/node is released a PR is created.

Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I have a question, but it is non-blocking.

@@ -34,7 +34,7 @@ jobs:
- name: Update Node version
working-directory: extensions/ql-vscode
run: |
npx ts-node scripts/update-node-version.ts
npx vite-node scripts/update-node-version.ts
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the benefit of using vite-node over ts-node?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vite-node has better support for mixed ESM/CJS modules, so I switched all other scripts to use vite-node in 85abd9a. I noticed I forgot to update this use, so this just makes it consistent with the other scripts which are defined in package.json.

@koesie10 koesie10 merged commit e3c79e4 into main Nov 7, 2024
16 checks passed
@koesie10 koesie10 deleted the koesie10/fix-update-node-version branch November 7, 2024 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants