Upgrade electron from 13 to 15 #1764
Conversation
efb4f5ff-1298-471a-8973-3d47447115dc
added
PR: WIP
electron being electron
PikachuEXE
force-pushed
the
upgrade/electron-13-to-15
branch
from
f454e48
to
db6acff
Compare
|
For #1651 Screen.Recording.2021-10-05.at.4.34.24.PM.movToo lazy to try for electron 14 |
|
I would opt for going with v 15 if nothing breaks ofcourse. Good to see that issue is also fixed. |
|
What other issues are left which are potentially linked to electron upgrade? |
|
I'm not sure anymore i thought there was another one but cant seem to remember. I've also tested it and looks good. I think its save to say that u can add closes #1651 to the PR. |
|
Added |
|
Oh oops i didnt see ur last comment. Well... This isnt going to be included in v0.15.0 anyway😁 |
|
Maybe this also fixes #1710. Need to look into that. I dont have a pinephone tho |
| webSecurity: false, | ||
| backgroundThrottling: false, | ||
| contextIsolation: false | ||
| }, | ||
| show: false | ||
| } | ||
| } | ||
| const newWindow = new BrowserWindow( | ||
| Object.assign( | ||
| { | ||
| // It will be shown later when ready via `ready-to-show` event | ||
| show: false | ||
| }, | ||
| commonBrowserWindowOptions | ||
| ) | ||
| ) | ||
|
|
||
| // region Ensure child windows use same options since electron 14 | ||
|
|
||
| // https://github.com/electron/electron/blob/14-x-y/docs/api/window-open.md#native-window-example | ||
| newWindow.webContents.setWindowOpenHandler(() => { | ||
| return { | ||
| action: 'allow', | ||
| overrideBrowserWindowOptions: Object.assign( | ||
| { | ||
| // It should be visible on click | ||
| show: true | ||
| }, | ||
| commonBrowserWindowOptions | ||
| ) | ||
| } | ||
| }) | ||
|
|
||
| // endregion Ensure child windows use same options since electron 14 | ||
|
|
||
| if (replaceMainWindow) { | ||
| mainWindow = newWindow | ||
| } |
There was a problem hiding this comment.
Is all of this even necessary? The app works fine without this since we don't use window.open() anywhere.
While I understand the argument of making it future-proof, it feels a bit unnecessary to do this since we're
probably never gonna use window.open() in Electron renderer windows.
There are a couple of reasons for this:
- the url would always have to be a path to the app, we'd have to prohibit everything else
- different types of packaging could possibly interfere with how
window.open()would behave, making it have
odd behaviors
This is just my two cents when I first saw this, not really a demand for a change.
Point is, as it stands now, the setWindowOpenHandler callback is not being used for anything because we create
"sibling" windows rather than child windows.
There was a problem hiding this comment.
This is actually required to fix #1636 which is caused by the breaking change introduced in Electron 14
Feel free to try a local version without this code
I might try it again later
There was a problem hiding this comment.
I forgot about that, that does use window.open()
I always considered being able to open a window in the app like that a bug rather than a feature, but I understand that some people actually like it
We will eventually have to get rid of it though, because right now, every window reloads the subscriptions feed which can lead to rate limiting if you are a heavy multi window user
Once we get rid of that, we'll have to disallow that particular window.open() behavior
This is fine for now, though
There was a problem hiding this comment.
If you mean opening a new window for a video in a slibing window instead of a child window, I would support that and even try to implement it myself.
Otherwise I am not sure what you mean.
|
As mentioned by @PrestonN in IRC Will update description too
|
|
Closes #1843, #1664 after this is fixed electron/electron#31448 |
|
Just not sure what versions will that fix land on... |
|
Ur right but just wanted to give u a headsup |
Breaking change: electron/electron#28550 Child windows no longer inherit BrowserWindow construction options from their parents.
PikachuEXE
force-pushed
the
upgrade/electron-13-to-15
branch
from
db6acff
to
334eae5
Compare
|
I'm going to go ahead and bypass our typical review process and go ahead and merge this in. The fixes from upgrading Electron are important to our next patch release and I'd really like to include these. I was able to do a quick test with this PR and it seems to fix all of the mentioned issues with the latest Electron patch. The PiP issue actually has to do with Chromium and the only way to fix that is to update Electron itself (Which we did obviously). Thanks again for getting this together! |
|
👍 |
efb4f5ff-1298-471a-8973-3d47447115dc
added
PR: dependencies
Blocking
Pull Request Type
Please select what type of pull request this is:
Related issue
Closes #1640
Closes #1651
Closes #1763
Description
Upgrade electron 13.x > 15.x AND fix the issue caused by a breaking change in electron 14
Breaking change:
electron/electron#28550
Child windows no longer inherit BrowserWindow construction options from their parents.
Screenshots (if appropriate)
Nothing visible should really changed
Testing (for code that is not small enough to be easily understandable)
All Dev team members should test this in
npm run dev) and built version afterward (can trigger nightly build manually)Desktop (please complete the following information):
Additional context
Upgrade to electron 14 had caused issue like #1636
Upgrade to electron 15 to see if it's fixed
(since our PiP implementation relies on
video.js> Electron > Chromium)References:
This might replace #1763