Update Node.js, Electron and node-pty #39296
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
avatus
approved these changes
Mar 13, 2024
ravicious
reviewed
Mar 13, 2024
| "start-electron": "electron build/app/dist/main/main.js", | ||
| "build": "electron-vite build", | ||
| "build-native-deps": "electron-builder install-app-deps", |
There was a problem hiding this comment.
Not depending on nan and not having to rebuild binaries each time that the Electron version changes is huge.
I don't have the time to thoroughly review this today, but what I think the switch to Node-API means is that node-pty is not bound to a particular Node version but instead works with any Node that supports Node-API, see:
I suppose that's why it works without manually rebuilding the native deps.
It's fine that electron-builder rebuilds it during packaging. This way we're always sure that the node-pty version shipped with the app was built with the same Node version that's going to be used by the app.
gzdunek
requested review from
klizhentas,
russjones,
r0mant,
zmb3,
fheinecke,
camscale and
tcsc
as code owners
|
I had to bump The commit 3054c8a didn't actually generate the lockfile for 0.9.0, it only switched to a newer Node.js package. |
ravicious
approved these changes
Mar 18, 2024
hugoShaka
reviewed
Mar 18, 2024
ravicious
reviewed
Mar 19, 2024
gzdunek
force-pushed
the
gzdunek/update-node-electron-node-pty
branch
from
cdf1453
to
7aa7b15
Compare
camscale
approved these changes
Mar 19, 2024
gzdunek
added a commit
that referenced
this pull request
Mar 19, 2024
* Update Node.js to 20.11.1 * Update Electron to 29.1.1 * Update node-pty to 1.1.0-beta12 * Remove commands for building native deps manually * Remove file skipping in electron-builder that seems to do nothing * Regenerate `devbox.lock` using the same Devbox version as the CI (0.9.0) * Update `devbox` to `0.9.1` * Update `README.md` * Update electron to 29.1.4 * Update devbox checksum * Remove mention of Node.js version in teleterm/README.md (cherry picked from commit 3140ad8)
gzdunek
added a commit
that referenced
this pull request
Mar 21, 2024
* Update Node.js to 20.11.1 * Update Electron to 29.1.1 * Update node-pty to 1.1.0-beta12 * Remove commands for building native deps manually * Remove file skipping in electron-builder that seems to do nothing * Regenerate `devbox.lock` using the same Devbox version as the CI (0.9.0) * Update `devbox` to `0.9.1` * Update `README.md` * Update electron to 29.1.4 * Update devbox checksum * Remove mention of Node.js version in teleterm/README.md (cherry picked from commit 3140ad8)
gzdunek
added a commit
that referenced
this pull request
Mar 21, 2024
* Update Node.js to 20.11.1 * Update Electron to 29.1.1 * Update node-pty to 1.1.0-beta12 * Remove commands for building native deps manually * Remove file skipping in electron-builder that seems to do nothing * Regenerate `devbox.lock` using the same Devbox version as the CI (0.9.0) * Update `devbox` to `0.9.1` * Update `README.md` * Update electron to 29.1.4 * Update devbox checksum * Remove mention of Node.js version in teleterm/README.md (cherry picked from commit 3140ad8)
gzdunek
added a commit
that referenced
this pull request
Mar 21, 2024
* Update Node.js to 20.11.1 * Update Electron to 29.1.1 * Update node-pty to 1.1.0-beta12 * Remove commands for building native deps manually * Remove file skipping in electron-builder that seems to do nothing * Regenerate `devbox.lock` using the same Devbox version as the CI (0.9.0) * Update `devbox` to `0.9.1` * Update `README.md` * Update electron to 29.1.4 * Update devbox checksum * Remove mention of Node.js version in teleterm/README.md (cherry picked from commit 3140ad8)
This was referenced Mar 21, 2024
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 21, 2024
* Update Node.js to 20.11.1 * Update Electron to 29.1.1 * Update node-pty to 1.1.0-beta12 * Remove commands for building native deps manually * Remove file skipping in electron-builder that seems to do nothing * Regenerate `devbox.lock` using the same Devbox version as the CI (0.9.0) * Update `devbox` to `0.9.1` * Update `README.md` * Update electron to 29.1.4 * Update devbox checksum * Remove mention of Node.js version in teleterm/README.md (cherry picked from commit 3140ad8)
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 21, 2024
* Update Node.js to 20.11.1 * Update Electron to 29.1.1 * Update node-pty to 1.1.0-beta12 * Remove commands for building native deps manually * Remove file skipping in electron-builder that seems to do nothing * Regenerate `devbox.lock` using the same Devbox version as the CI (0.9.0) * Update `devbox` to `0.9.1` * Update `README.md` * Update electron to 29.1.4 * Update devbox checksum * Remove mention of Node.js version in teleterm/README.md (cherry picked from commit 3140ad8)
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 21, 2024
* Update Node.js to 20.11.1 * Update Electron to 29.1.1 * Update node-pty to 1.1.0-beta12 * Remove commands for building native deps manually * Remove file skipping in electron-builder that seems to do nothing * Regenerate `devbox.lock` using the same Devbox version as the CI (0.9.0) * Update `devbox` to `0.9.1` * Update `README.md` * Update electron to 29.1.4 * Update devbox checksum * Remove mention of Node.js version in teleterm/README.md (cherry picked from commit 3140ad8)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This time we have a bigger update :)
However, they upgraded Node.js to v20, so I did the same for us.
I checked if the supported systems requirements haven't changed, the only difference I see is for macOS; now 10.15 is required for x64. This aligns with Electron compatibility, starting from v28 they require v10.15 too.
Recently, the library has been ported to use Node-API. If I'm correct, this means that we no longer need to rebuild it manually, but instead the native code is compiled in
[4/4] 🔨 Building fresh packages...phase. I verified that on macOS and Windows, the app starts without a manual deps rebuild.I looked at the
yarn installtime on CI, it doesn't seem that there is any significant difference.Additionally, the package no longer depends on
nan(it is predecessor of NAPI), which in the past we had to update along with Electron.I removed all commands that we used to build deps natively and the code related to
node-ptythat we had inelectron-builder-config.js- it seems that these files doesn't exist anyway.Note:
electron-builderstills rebuildsnode-ptywhen packaging the app, I don't know if this is necessary now, but AFAIK we can't turn it off. I hope I'm not wrong about that Node-API stuff :) I learned more about it from here.Tag build: 15.0.0-dev.gzdunek.12, tested on macOS, Windows, Ubuntu.
Changelog: Updated Electron to v29 in Teleport Connect