Upgrade to electron 22 - Githubissues

jeffvli / sonixd

A full-featured Subsonic/Jellyfin compatible desktop music player

GNU General Public License v3.0

1.87k stars 73 forks source link

Upgrade to electron 22 #397

Closed kgarner7 closed 1 year ago

kgarner7 commented 2 years ago

I know there's work on a rewrite, but while this is still here, I'd like to at least provide updated packages.

Changes:

electron -> 21
- Use ipcmain/renderer to send minimize/maximize messages; drop preload.ts
- use electron-store instead of electron-settings. This does introduce a dependency loop (setDefaultSettings -> i18n -> ...), but it should be OK because i18n is called later in settings
- to support testing electron store, new mock
- new electron-notarize to correct package (warning when running yarn)
update other packages as possible

NOTE: I had to drop NodeRT because I could not get it to build on windows. I tried to use MediaSession (which is supposed to show notifications), but windows is still confused .-.

jeffvli commented 2 years ago

I appreciate the work to keep Sonixd up to date!

Couple initial thoughts:

Is there a reason for electron-store over electron-settings? My assumption for this change is that electron-settings breaks with Electron 21?
I'd really like to keep NodeRT (or at least have a working alternative). I do know of some users who make use of NodeRT's Windows media API, so that would be a breaking change if removed.

I can do a bit of testing on my end to see if I can get it working once I get a chance!

kgarner7 commented 2 years ago

The reason for using electron-store actually relates to an upgrade from electron 13 -> 14 (https://www.electronjs.org/blog/electron-14-0/). Notably, the removal of the remote module. Unfortunately, electron-settings relies heavily on this (and using it as-is breaks).

Yes, it is possible to re-enable remote, but given that electron-settings hasn't been updated a little over 2 years, it seemed like using a different store that actually uses IPC would be better.

If you can confirm that NodeRT still works (or share how to get it properly set up), that would be lovely! I only have win11 (and had to zap my prior win 10 VM that was able to build sonixd...)

jugendhacker commented 2 years ago

This will also close #393

jeffvli commented 1 year ago

@kgarner7 I'm going to attempt to merge this in. I pushed a PR over to your fork to re-add in the nodert dependencies since they still build fine on the publish pipelines. Seems like only the tests are failing.

kgarner7 commented 1 year ago

Done! Glad to see sonixd still getting some love