Skip to content

Conversation

@erikian
Copy link
Member

@erikian erikian commented Jan 11, 2025

BREAKING CHANGE: bumps required Node.js version to >=22.12.0. ESM-only.

@electron electron deleted a comment from notion-workspace bot Jan 18, 2025
@erikian erikian force-pushed the feat/node-22 branch 3 times, most recently from 24c032a to 7233aff Compare April 7, 2025 12:13
@erikian erikian marked this pull request as ready for review April 8, 2025 01:54
@erikian erikian requested a review from a team as a code owner April 8, 2025 01:54
package.json Outdated
"prepare": "tsc"
},
"dependencies": {
"commander": "^5.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

Would love to upgrade commander in a follow-up PR to align the major versions across all Electron tools that have multiple commands and still need a lib like commander.

Copy link
Member Author

Choose a reason for hiding this comment

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

It only needs an import to be updated, so we can probably just do it in this PR - 0be538d

Comment on lines +1 to +5
import { createRequire } from 'node:module';

const require = createRequire(import.meta.url);

const fs = 'electron' in process.versions ? require('original-fs') : require('node:fs');
Copy link
Member

Choose a reason for hiding this comment

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

Note: this ESM/CommonJS interop thing reminds me that we should test that CommonJS aps run smoothly when upgrading @electron/asar to the next major version in Electron core.

Since require(esm) comes with Node 22, we should be okay in the newest versions of Electron core... right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had to update electron to ^34.5.0 (Node 20.19.0) in our devDependencies because we run our Mocha tests on Electron through electron-mocha and that's the first version where require(esm) works without the --experimental-require-module flag — we get ERR_REQUIRE_ESM on lower versions, though we could use Electron 32.1.0 (Node 20.17.0) and up by changing our mocha script to:

"mocha": "NODE_OPTIONS=\"--experimental-require-module\" xvfb-maybe electron-mocha && mocha",

So to answer your question, I think Electron core should be able to upgrade to @electron/asar@^4.0.0 as soon as Electron 36 is out and Electron 34 becomes the oldest supported version (it still uses Node 20.x — if we want to wait for Node 22.x we'd need to wait for Electron 37 to be released).

@erikian erikian requested review from erickzhao and mmaietta April 12, 2025 17:14
@erikian erikian force-pushed the feat/node-22 branch 2 times, most recently from 1a6eb23 to 7fa2c75 Compare April 13, 2025 15:57
@dsanders11
Copy link
Member

@erikian, since this PR has been open for a while, can you merge in main to ensure tests still pass?

"noImplicitAny": true,
"strictNullChecks": true,
"esModuleInterop": true
"noUnusedLocals": true
Copy link
Member

Choose a reason for hiding this comment

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

nit: i think @typescript-eslint/no-unused-vars already covers this case?

@dsanders11 dsanders11 merged commit c14d903 into main May 14, 2025
4 checks passed
@dsanders11 dsanders11 deleted the feat/node-22 branch May 14, 2025 21:24
@continuous-auth
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.

6 participants