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

chore(app,app-shell,app-shell-odd, protocol-designer, labware-library, components, labware-designer): update Electron version to v27.0.0 #14314

Merged
merged 46 commits into from
Jan 31, 2024

Conversation

koji
Copy link
Contributor

@koji koji commented Jan 12, 2024

Overview

Update Electron version 27.0.0
https://www.electronjs.org/docs/latest/tutorial/electron-timelines#timeline

There is loadURL issue in the versions that include the following PR.
electron/electron#40143

At this moment we cannot update any newer version. (I'll keep checking the repo)

This PR also includes Nodejs update because of Electron v27's requirement.
This PR needs Nodejs v21.4.0 since M1 Mac has an issue about running Desktop app. (I'll keep an eye on this to support more nodejs version). M2 Mac can use nodejs v18.

Need NODE_OPTION due to OpenSSL version.
I tried to export NODE_OPTION in the top of Makefile, but somehow that has an impact on other part and prevent building the app(Desktop/ODD). I use NODE_OPTION separately.
Another reasons for this are labware-library and protocol-designer since the option prevents Cypress.

Running the application

node v21

  • macOS
  • Windows
  • Ubuntu
  • ODD mode
  • ODD

CI

  • update Nodejs version (v16 -> v21)

OT-2

  • USB connection

Other applications

  • Protocol Designer
  • Labware Library
  • Labware Designer
  • Storybook

Commands

  • make check-js
  • make lint-js
  • make test-js

This PR removes usb-detection since the repo has been archived and not compatible with node v18+.
The new package we need to use is [node-usb](https://github.com/node-usb/node-usb)
https://working-wormhole-d95.notion.site/usb-detection-node-usb-a6637f72f5d44f3ba9c7520b4f0da24f

Test Plan

  • Run Desktop app and check that the app works as well as the current edge
  • Run ODD app and check that the app works as well as the current edge

Changelog

Review requests

Risk assessment

@koji koji requested a review from a team as a code owner January 12, 2024 15:58
@koji koji requested a review from a team January 12, 2024 15:58
@koji koji requested a review from a team as a code owner January 12, 2024 15:58
@koji koji requested review from smb2268 and removed request for a team January 12, 2024 15:58
@koji koji marked this pull request as draft January 12, 2024 15:58
@koji koji removed request for a team and smb2268 January 12, 2024 15:59
Copy link

codecov bot commented Jan 12, 2024

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (ffcc548) 68.29% compared to head (d559db9) 68.26%.
Report is 14 commits behind head on edge.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #14314      +/-   ##
==========================================
- Coverage   68.29%   68.26%   -0.04%     
==========================================
  Files        1623     2514     +891     
  Lines       54858    72017   +17159     
  Branches     4115     9242    +5127     
==========================================
+ Hits        37466    49163   +11697     
- Misses      16705    20678    +3973     
- Partials      687     2176    +1489     
Flag Coverage Δ
app 64.93% <41.46%> (+30.09%) ⬆️
components 49.62% <ø> (ø)
g-code-testing 96.48% <ø> (ø)
labware-library 41.10% <ø> (ø)
protocol-designer 38.01% <ø> (ø)
react-api-client 66.16% <ø> (ø)
shared-data 75.25% <ø> (ø)
step-generation 86.90% <ø> (ø)
update-server 63.58% <ø> (ø)
usb-bridge 76.94% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
.../src/organisms/AdvancedSettings/U2EInformation.tsx 100.00% <ø> (ø)
app/src/redux/system-info/selectors.ts 90.90% <ø> (ø)
robot-server/robot_server/runs/run_store.py 100.00% <ø> (ø)
app-shell-odd/src/main.ts 0.00% <0.00%> (ø)
app-shell/src/system-info/index.ts 84.21% <80.00%> (-5.79%) ⬇️
app-shell/src/system-info/usb-devices.ts 75.00% <55.55%> (-9.62%) ⬇️
app-shell-odd/src/ui.ts 0.00% <0.00%> (ø)
app-shell/src/ui.ts 0.00% <0.00%> (ø)
app-shell/src/main.ts 0.00% <0.00%> (ø)

... and 893 files with indirect coverage changes

@koji koji changed the title chore(app,app-shell,app-shell-odd): update Electron version to v23 chore(app,app-shell,app-shell-odd, protocol-designer, labware-library, components, labware-designer): update Electron version to v27.0.0 Jan 12, 2024
@sfoster1
Copy link
Member

Looks like flex builds will fail until we get node-native on >=18 https://github.com/Opentrons/oe-core/actions/runs/7659498340/job/20874747633

@sfoster1
Copy link
Member

Tracking the oe-core work here Opentrons/oe-core#128

@sfoster1
Copy link
Member

@sfoster1
Copy link
Member

And it works, happy with this from the ODD side

@koji
Copy link
Contributor Author

koji commented Jan 26, 2024

  • try v18.19.0 again
  • solve conflicts
  • fix CI errors

@brenthagen
Copy link
Contributor

I'm getting some strange behavior on this branch from locally simulated robots - reporting as unreachable but responding with 200s to various requests. I'm not sure why, digging a bit further.

this is an issue with node-fetch and dns default result order https://opentrons.slack.com/archives/CSCLVUW3C/p1706568971026189

i've pushed a commit to fix

* setting the default to IPv4 fixes the issue
* https://github.com/node-fetch/node-fetch/issues/1624
*/
// TODO(bh, 2024-1-30): @types/node needs to be updated to address this type error. updating @types/node will also require updating our typescript version
Copy link
Contributor

Choose a reason for hiding this comment

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

this is failing because our wildcard @types/node version is 15 in yarn.lock. i'm looking into updating our wildcard @types/node version and typescript in a separate commit.

brenthagen and others added 3 commits January 30, 2024 17:50
2a360ce tells node to prefer ipv4
addresses since in node >17 it prefers ipv6. Unfortunately, when we're
running cypress we don't really have the ability to specify this since
it happens in code. Instead, we can make sure that webpack-dev-server
serves on ipv6 as well as ipv4; the right way to do this is to tell it
to use the host :: which is ipv6 shorthand for "accept-all-incoming" in
the same way that 0.0.0.0 is for ipv4.

This should actually accept all incoming v4 requests too. You can test
it by running make dev and then doing both
curl --head 127.0.0.1:8080/ (the ipv4 version)
curl --head '[::1]:8080/' (the ipv6 version; ::1 is ipv6 localhost, and
the brackets are how ipv6 is written. if doing this in the shell you'll
need single-quotes here)
curl --head http://localhost:8080/
node ./renderStatic.js

# development assets server
.PHONY: dev
dev: export NODE_ENV := development
dev:
webpack-dev-server --hot
export NODE_OPTIONS=--openssl-legacy-provider && webpack-dev-server --hot --host=::
Copy link
Member

Choose a reason for hiding this comment

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

the other side of the ipv4/v6 thing is that it also applies to cypress runs, and there we can't run our own js code, so let's just serve on v6 wildcard. you can test this by doing curl --head '[::1]:8080/' (note the single quotes - your shell will try and parse those brackets) when dev server is running, as well as curl --head 127.0.0.1:8080/ and curl --head http://localhost:8080/

Copy link
Contributor

Choose a reason for hiding this comment

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

think we need this in app also, i get same behavior locally on node 18 (but not 20 it seems).

and i guess labware-designer while we're at it

@sfoster1
Copy link
Member

sfoster1 commented Jan 31, 2024

  • fix ci errors

and twiddle some env exports a bit
Copy link
Member

@sfoster1 sfoster1 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 to me! @koji I think you should have the honor of clicking merge!

@sfoster1 sfoster1 merged commit 3308ae5 into edge Jan 31, 2024
66 of 69 checks passed
@sfoster1 sfoster1 deleted the chore_update-electron-v27.0.0 branch January 31, 2024 22:17
ncdiehl11 pushed a commit that referenced this pull request Feb 1, 2024
…omponents,labware-designer): update Electron version to v27.0.0 (#14314)

Update Electron version 27.0.0
https://www.electronjs.org/docs/latest/tutorial/electron-timelines#timeline

This is the latest version of electron that does not include electron/electron#40143 which breaks the ability of the ODD to load content.

This PR also includes Nodejs update because of Electron v27's requirement.

The more recent nodejs requires some new options; specifically, webpack dev servers must serve on ipv6 wildcards, and internally in the shell we need to prefer v4 dns. We also need to use old openssl.

---------

Co-authored-by: Brent Hagen <[email protected]>
Co-authored-by: Seth Foster <[email protected]>
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.

4 participants