chore: upgrade electron to v22 #720

achou11 · 2023-02-08T20:13:17Z

Towards #715

Changes:

Upgrade Electron to 22.3.2
- Requires migration to @electron/remote
  - Requires changing usage of electron-is-dev because it no longer works in the renderer process as a result of remote removal
- Upgrade map server to allow usage of better-sqlite3 > 7, which is required for this version of Electron
Upgrade Node to 16.17.1 for dev environment, to match the version that Electron runs
- Patches ecstatic because of invalid shebang in source file
Upgrade react-intl to 6.2.8
- Not sure if this was necessary but based on Upgrage Electron and Bundler #715 (comment), seemed necessary. (not sure how or if that note was related to Electron specifically though)
- Can potentially revert this since it may be out of scope for the electron upgrade
Fixes race condition issue with map server initialization when app starts for the first time
Small removals/cleanups for unused things for development

Questions:

Is it okay to use npm 8 instead of 6? how does that affect native builds if at all?
What changes are needed in build.config.js to get things working? Seems like we do a lot of manual file picking in there so may need to update based on deps changes
Should we revert upgrade of react-intl if possible? (see above for details)

Potential changes to make (either here or in a follow-up):

Complete migration away from using remote apis.
Improve usage of files that are used in different processes (e.g. store.js, client-ipc.js, logger.js, etc)

achou11 · 2023-02-08T20:16:44Z

config.js

+  getResourcesDir,
+  getDefaultConfigDir


replaced the statically defined constants with these functions because this file is run in various contexts that are sensitive to the environment e.g. main process, background renderer, primary window renderer

achou11 · 2023-02-08T20:18:13Z

src/background/index.js

 const label = path.basename(modulePath)

+// TODO: Used by renderer processes to determine if in development mode or not. There's probably a better way to do this
+window.mode = mode


window.mode is either "development" or "production" and is defined by a value passed to webPreferences.additionalArguments when creating a BrowserWindow in the main process

achou11 · 2023-02-08T20:24:12Z

src/main/index.js

+    mapServer = createMapServer(undefined, {
+      database: new Database(path.join(mapsdir, 'maps.db'))
+    })


maybe out of scope, but wondering if this should point to temp-resources when in development, instead of the user data directory

yeah some kind of tmp directory would make sense. might be good to make an issue for it rather than deal with it in this pr.

yeah we already create a temp-resources which includes some assets like the presets. just a matter of whether it makes sense to put the maps db here during dev too

achou11 · 2023-02-08T21:49:55Z

seem to be running into an issue with rebuilding native deps during electron-build in CI. link

Not entirely sure if it's because of using Node 16 + npm 8 or if it's an issue with a dep. Potential culprit dep seems to be [email protected], which seems to only be a dev dep (or at least only used by dev deps):

2023-02-08T20:36:34.5514150Z     npm ERR! In file included from ../fsevents.cc:6:
2023-02-08T20:36:34.5615460Z     npm ERR! In file included from ../../nan/nan.h:174:
2023-02-08T20:36:34.5716810Z     npm ERR! ../../nan/nan_callbacks.h:55:23: error: no member named 'AccessorSignature' in namespace 'v8'
2023-02-08T20:36:34.5817060Z     npm ERR! typedef v8::Local<v8::AccessorSignature> Sig;
2023-02-08T20:36:34.5918410Z     npm ERR!                   ~~~~^
2023-02-08T20:36:34.6018810Z     npm ERR! In file included from ../fsevents.cc:6:
2023-02-08T20:36:34.6120980Z     npm ERR! ../../nan/nan.h:2536:8: error: no matching member function for call to 'SetAccessor'
2023-02-08T20:36:34.6222610Z     npm ERR!   tpl->SetAccessor(
2023-02-08T20:36:34.6325020Z     npm ERR!   ~~~~~^~~~~~~~~~~
2023-02-08T20:36:34.6427110Z     npm ERR! /Users/runner/.electron-gyp/22.2.0/include/node/v8-template.h:814:8: note: candidate function not viable: no known conversion from 'imp::Sig' (aka 'int') to 'v8::SideEffectType' for 7th argument
2023-02-08T20:36:34.6527000Z     npm ERR!   void SetAccessor(
2023-02-08T20:36:34.6628160Z     npm ERR!        ^
2023-02-08T20:36:34.6730350Z     npm ERR! /Users/runner/.electron-gyp/22.2.0/include/node/v8-template.h:807:8: note: candidate function not viable: no known conversion from 'imp::NativeGetter' (aka 'void (*)(v8::Local<v8::Name>, const v8::PropertyCallbackInfo<v8::Value> &)') to 'v8::AccessorGetterCallback' (aka 'void (*)(Local<v8::String>, const PropertyCallbackInfo<v8::Value> &)') for 2nd argument
2023-02-08T20:36:34.6829690Z     npm ERR!   void SetAccessor(
2023-02-08T20:36:34.6930900Z     npm ERR!        ^
2023-02-08T20:36:34.7032310Z     npm ERR! In file included from ../fsevents.cc:6:
2023-02-08T20:36:34.7132950Z     npm ERR! In file included from ../../nan/nan.h:2884:
2023-02-08T20:36:34.7234660Z     npm ERR! ../../nan/nan_typedarray_contents.h:34:43: error: no member named 'GetContents' in 'v8::ArrayBuffer'
2023-02-08T20:36:34.7321430Z     npm ERR!       data   = static_cast<char*>(buffer->GetContents().Data()) + byte_offset;

wonder if there's a way to tell electron-builder to omit that (see relevant rebuild options here). @gmaclennan maybe you have an idea?

gmaclennan · 2023-02-08T21:53:39Z

There should be a way to omit builds for that? If everything is available as prebuild, then you can turn off build from source. But I haven't really got into the weeds of electron rebuilds so don't know a solution off the top of my head.

in ci, electron build attempts to rebuild [email protected], which is only used as a dev dep. should potentially be okay to omit

achou11 · 2023-02-09T18:23:49Z

There should be a way to omit builds for that? If everything is available as prebuild, then you can turn off build from source. But I haven't really got into the weeds of electron rebuilds so don't know a solution off the top of my head.

for posterity, added fsevents as one of the native deps to remove in the remove-native-deps npm script (39fabc8). let's see if that works fine for our case...

gmaclennan

Looks ok to me. Frustrating that Electron keeps adding restrictions that makes simple stuff more complicated. If this works ok (I haven't tested it) then good to merge I think,.

ErikSin and others added 16 commits January 30, 2023 02:23

chore: upgraded electron and node

9076ec2

bring back map server using latest

aa774f2

replace electron.remote with @electron/remote

3184ed1

fix usage of electron-is-dev module

6ec4819

add patch for ecstatic

ba81279

remove duplicate code in app.js

5ce26c8

fix import of electron remote in renderer

5248e38

remove electron remote from externals

3d06334

fix issue with creating map server db before directory exists

b1c0c25

fix mapServer scoping error when quitting app

5cc4c5b

upgrade react-intl to remove usage of broken peer dep

364241b

remove unused livereload functionality

d92a821

remove commented-out webpack plugin

536c002

update comment in new api file

c548a1a

adjustments for window.mode in background windows

9ea06e0

upgrade electron to 22.2.0

d78ae70

achou11 commented Feb 8, 2023

View reviewed changes

achou11 added 2 commits February 8, 2023 15:26

ci changes

4278c40

Merge branch 'master' into es/chore-electron-upgrade

1d62322

achou11 added 4 commits February 8, 2023 17:00

maybe fix electron build issue with fsevents

39fabc8

in ci, electron build attempts to rebuild [email protected], which is only used as a dev dep. should potentially be okay to omit

maybe fix rimraf issue on windows ci

d230364

partially undo previous

666ba8f

try node script for ci artifact removal

9e9e37d

achou11 requested review from ErikSin and gmaclennan February 9, 2023 18:33

achou11 marked this pull request as ready for review February 9, 2023 18:33

upgrade map server to latest

461bf68

achou11 added 2 commits February 9, 2023 16:59

upgrade electron to latest 22

3f98e7d

update babel config

41f5a65

achou11 mentioned this pull request Feb 16, 2023

chore: remove Flow from project #721

Merged

achou11 added 2 commits February 28, 2023 11:52

Merge branch 'master' into es/chore-electron-upgrade

c730f47

update electron to latest v22

709fa4d

gmaclennan approved these changes Mar 8, 2023

View reviewed changes

upgrade electron to v22.3.2

fa7bd87

achou11 merged commit 79c0f1f into master Mar 8, 2023

achou11 deleted the es/chore-electron-upgrade branch March 8, 2023 20:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: upgrade electron to v22 #720

chore: upgrade electron to v22 #720

achou11 commented Feb 8, 2023 •

edited

Loading

achou11 Feb 8, 2023

achou11 Feb 8, 2023

achou11 Feb 8, 2023 •

edited

Loading

sethvincent Feb 8, 2023

achou11 Feb 8, 2023 •

edited

Loading

achou11 commented Feb 8, 2023

gmaclennan commented Feb 8, 2023

achou11 commented Feb 9, 2023 •

edited

Loading

gmaclennan left a comment

chore: upgrade electron to v22 #720

chore: upgrade electron to v22 #720

Conversation

achou11 commented Feb 8, 2023 • edited Loading

achou11 Feb 8, 2023

Choose a reason for hiding this comment

achou11 Feb 8, 2023

Choose a reason for hiding this comment

achou11 Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

sethvincent Feb 8, 2023

Choose a reason for hiding this comment

achou11 Feb 8, 2023 • edited Loading

Choose a reason for hiding this comment

achou11 commented Feb 8, 2023

gmaclennan commented Feb 8, 2023

achou11 commented Feb 9, 2023 • edited Loading

gmaclennan left a comment

Choose a reason for hiding this comment

achou11 commented Feb 8, 2023 •

edited

Loading

achou11 Feb 8, 2023 •

edited

Loading

achou11 Feb 8, 2023 •

edited

Loading

achou11 commented Feb 9, 2023 •

edited

Loading