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

feat!: bump engines to Node.js >=22.12.0 #312

Merged
merged 20 commits into from
Feb 18, 2025
Merged

feat!: bump engines to Node.js >=22.12.0 #312

merged 20 commits into from
Feb 18, 2025

Conversation

erickzhao
Copy link
Member

@erickzhao erickzhao commented Jan 11, 2025

BREAKING CHANGE: bumps minimum Node.js version to >=22.12.0

ESM

Node.js 22.12 gives us the ability to use CommonJS require calls on ESM dependency graphs. This compatibility gives us the ability to migrate our libraries to ESM and maintain support for projects that are using CommonJS.

Removal of deprecated APIs

@electron/get already had a few deprecated APIs and this PR removes them since we're already making a breaking change anyways:

  • nightly_mirror option was removed (was deprecated in favour of nightlyMirror)
  • force option was removed (was deprecated in favour of cacheMode=WriteOnly).

Test migration from Jest to Vitest

Jest currently has issues with require(esm) while it works out of the box with Vitest. We were already looking to migrate to Vitest across other Ecosystem repos, so this PR just switches over.

  • Need to import functions from vitest (no globals)
  • jest.mock calls turned into vi calls
  • Regenerated snapshots

tsconfig.json changes

We're now basing our TypeScript config off of @tsconfig/node22, which is a bit different from what we had in the past.

Due to going ESM-only, we only have a singular tsconfig.json now.

d('* Replacing existing file');
await fs.remove(cachePath);

if (!fs.existsSync(path.dirname(cachePath))) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: this new clause is needed because fs.rename will fail if the target directory doesn't exist, while fs.move creates the directory.

This also doesn't work across disks, but we could catch the resulting error and fall back to an fs.copy call if necessary.

@erickzhao erickzhao marked this pull request as ready for review January 13, 2025 21:49
@erickzhao erickzhao requested a review from a team as a code owner January 13, 2025 21:49
@erickzhao erickzhao requested a review from dsanders11 January 20, 2025 18:26
@erickzhao erickzhao changed the title feat!: bump engines to Node.js 22 feat!: bump engines to Node.js >=22.12.0 Jan 22, 2025
@@ -109,8 +111,7 @@ describe('artifact-utils', () => {
artifactName: 'electron',
mirrorOptions: {
mirror: 'https://mirror.example.com/',
// eslint-disable-next-line @typescript-eslint/camelcase
Copy link
Member

Choose a reason for hiding this comment

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

Thank god

Copy link
Member

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

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

LGTM, other than the declaration: true change in tsconfig.json that @erikian called out.

@erickzhao
Copy link
Member Author

erickzhao commented Feb 11, 2025

0f540cd adds declarations back and uses util.promisify because graceful-fs unfortunately doesn't patch fs.promises as per isaacs/node-graceful-fs#160

@dsanders11
Copy link
Member

and uses util.promisify because graceful-fs unfortunately doesn't patch fs.promises

Sad, but good catch!

Copy link
Member

@VerteDinde VerteDinde left a comment

Choose a reason for hiding this comment

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

Excited to get this landed!

@erickzhao erickzhao merged commit 8be1f8d into main Feb 18, 2025
5 checks passed
@erickzhao erickzhao deleted the node-22 branch February 18, 2025 22:49
Copy link

🎉 This PR is included in version 4.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants