Conversation
|
I'm not sure how to update/interact with You can see that the first contains some kind of versioning/hash for nodejs 18.20.4: FROM node:18.20.4@sha256:e9ad817b0d42b4d177a4bef8a0aff97c352468a008c3fdb2b4a82533425480df |
|
I don't think it's necessary that (I could imagine it's necessary if for instance you use this library in other contexts (beyond just as a submodule for Quiet) that I'm not aware of) |
|
I'll look at these failing e2e tests |
I updated the dockerfile with the new node docker image. I also added a comment with the dickerhub page for future reference because I had to reorient myself with what this was. |
|
So in order to go from electron 23 to 29 (first version that uses node 20) we had to replace assignments of import { Crypto } from '@peculiar/webcrypto'
const webcrypto = new Crypto()
global.crypto = webcryptowith import { Crypto } from '@peculiar/webcrypto'
const webcrypto = new Crypto()
Object.defineProperty(global, 'crypto', {
value: webcrypto,
writable: true,
})
I kept things this way to preserve what the app was doing, and this works... @islathehut It seems that nodejs 20 and electron's chromium internals both support the standard |
|
There are some new tests failing! Which I'm working on right now...But apparently electron 29 has a vulnerability - which means we have to upgrade all the way to electron 35+. This seems kind of involved, but can do. It may require recompiling the native libraries on desktop: Behavior Changed: Native modules now require C++20
**EDIT: ** fixed the failing tests .... **EDIT: ** this vulnerability actually doesn't impact quiet desktop... From: https://nvd.nist.gov/vuln/detail/CVE-2025-55305 "Electron is a framework for writing cross-platform desktop applications using JavaScript, HTML and CSS. In versions below 35.7.5, 36.0.0-alpha.1 through 36.8.0, 37.0.0-alpha.1 through 37.3.1 and 38.0.0-alpha.1 through 38.0.0-beta.6, ASAR Integrity Bypass via resource modification. This only impacts apps that have the Since Quiet Desktop doesn't include the Aside from being desirable to be on a newer, supported, version of electron, you'll eventually have to, for instance electron 35 is needed to use node 22. Also for context, the latest electron is 40, and the lowest supported version is 37. According to the official breaking changes guide
|
…ts. Fixes the e2e stuff that fails on GitHub now that electron has changed versions. Added this step to new nodejs upgrade guide doc
…ditional hardcoded nodejs version config, and makes it easier to upgrade nodejs in the future and to keep tests on dev machines and github CI running with same dependencies
mcginty
left a comment
There was a problem hiding this comment.
The only blocking change is that .nvmrc file in the workflow, otherwise it seems good with the caveat question about switching to global.crypto.
|
|
||
| The .nvmrc file in the root of the project specifies the node verison that is used. Update this file to the newest version of Node, which will automatically update the node version used by the GitHub action CI scripts found in the `./github` directory. | ||
|
|
||
| With [`nmv` installed](https://github.com/nvm-sh/nvm?tab=readme-ov-file#install--update-script) run: |
There was a problem hiding this comment.
| With [`nmv` installed](https://github.com/nvm-sh/nvm?tab=readme-ov-file#install--update-script) run: | |
| With [`nvm` installed](https://github.com/nvm-sh/nvm?tab=readme-ov-file#install--update-script) run: |
| - uses: actions/setup-node@master | ||
| with: | ||
| node-version: 18.20.4 | ||
| node-version-file: ".nmvrc" |
There was a problem hiding this comment.
| node-version-file: ".nmvrc" | |
| node-version-file: ".nvmrc" |
There was a problem hiding this comment.
oh whoops. idk how this worked on the CI here, i think it just defaulted to the correct spelling. I'll fix this
| Object.defineProperty(global, 'crypto', { | ||
| value: webcrypto, | ||
| writable: true, | ||
| }) |
There was a problem hiding this comment.
I understand from the above comments why this is here, in that you didn't want to swap out crypto implementations at the same time you upgraded Node.
Maybe we should put a TODO here unless you're planning on creating another PR directly after this to switch to the builtin global.crypto implementation?
|
I just pushed a commit with your feedback
From the Slack chat it seems that no one remembers for certain why this lib was used in place of the default one. I will next work on figuring out if it's actually bringing something to the table, and if it's not like i suspect i'll gut it and replace with the standard lib. Thanks for reviewing ! |
|
With the build re-triggered (I'm assuming it broke because of GitHub uptime issues), it looks like there are some CI issues to work out still. |
|
All set ! |
|
It looks like there's a vulnerability that CI isn't happy about in https://github.com/TryQuiet/quiet/actions/runs/22786699786/job/66104875820?pr=3113. |
|
This has been discussed on this PR: After talking in Slack, it seemed fine to merge this with the understanding that we'd eventually upgrade to electron 35+ |
|
Ah apologies, I missed that! |
In order to upgrade react native versions to support Android 16 (API 36) we need to be using at least nodejs v20. A more extensive update should be undertaken to update to at least node 22, however this requires rebuilding native modules on desktop, Android, and iOS. This is an effective stop gap for the time being.
Tested on Desktop and Mobile, and things seem to work fine.