ci: update to electron 17 #36

mjcctech · 2023-11-13T13:21:18Z

What

Update electron to version 17.

Why

To minimize vulnerabilities and keep project more up to date.

Code is prepared for electron 19 but integration tests support electron up to 17.

a4xrbj1 · 2023-11-13T13:47:34Z

Thanks @mjcctech for pushing this to version 19. I totally agree that such a jump would help us a lot with the known vulnerabilities of the current (max) version, however even the selected version 19.1.9 has a long way to go with the known vulnerabilities as per Snyk: https://security.snyk.io/package/npm/electron/19.1.9

But one step at a time, 19.1.9 would be great and I'm eager to try it out with my current app.

mjcctech · 2023-11-13T14:34:31Z

@a4xrbj1 Thanks for quick reaction. Yeah, for now electron 19 - it took me whole time I had now. Unfortunately integration tests fails again. I see Spectron is used there and is deprecated and there is no version for electron 19. https://github.com/electron-userland/spectron#-spectron-is-officially-deprecated-as-of-february-1-2022

a4xrbj1 · 2023-11-13T14:52:32Z

Unfortunately integration tests fails again. I see Spectron is used there and is deprecated and there is no version for electron 19. https://github.com/electron-userland/spectron#-spectron-is-officially-deprecated-as-of-february-1-2022

See the comments from @dd137 on Spectron: #24 - I think we need to get rid of it as it will stop us from upgrading further.

Any views/comments? I've never used it and I'm not sure how it's affecting the test scripts.

@StorytellerCZ - any opinion, Jan?

StorytellerCZ · 2023-11-13T15:02:19Z

I'm fine for getting rid of Spectron, but it would be better to replace it with something so that we don't loose the tests.

a4xrbj1 · 2023-11-13T15:21:38Z

I'm fine for getting rid of Spectron, but it would be better to replace it with something so that we don't loose the tests.

Bard lists 4 alternatives: https://g.co/bard/share/50097afb48b3

Here's an article on Medium which has the opinion that Playwright is the best for testing ElectronJS (having Microsoft behind it is certainly good): https://betterprogramming.pub/how-to-test-electron-apps-1e8eb0078d7b

This is from the Cypress homepage: https://docs.cypress.io/guides/guides/launching-browsers

https://www.npmjs.com/package/electron-playwright-helpers

However, this page https://playwright.dev/docs/api/class-electron says:

"Playwright has experimental support for Electron automation."

and that the supported version is 14+

I personally would give my vote to either Playwright (no experience) or Cypress (used them in the past but never with ElectronJS).

dd137 · 2023-11-14T09:14:13Z

Thanks @mjcctech!

Out of curiosity, any particular reason for using version 19 of Electron?
11 - version currently used by meteor-desktop (Node 12)
14 - last version using Node 14, same as Meteor 2.x's Node
17 - last version compatible with Spectron (spectron@19) (Node 16)
27 - latest version (Node 18)

It would be interesting to know if having Electron run a more recent version of Node than Meteor could be a problem. I suspect not, beyond users having to be careful which Node features they use in the main VS renderer processes, but I'm not sure.

As for the tests, I agree it'd be great to replace Spectron by something else. I experimented with (Selenium) WebDriver last month and I was able to make it work easily with Electron, so that's another good candidate. I don't have time to rewrite the whole test suite now unfortunately. I'd say the person who will should choose the framework. In the meantime, we could also use this PR to update to Electron 17 already (or 14, if the Node version is an issue), if that makes the Spectron tests work.

mjcctech · 2023-11-14T09:23:20Z

There is no particular reason for version 19. I strive to update it as much as I can. If I have time I will check more recent Electron version.

It's fine for me to use Electron 17 in this merge request if integration test pass. Will check that.

a4xrbj1 · 2023-11-14T09:46:30Z

Thanks @mjcctech!

Out of curiosity, any particular reason for using version 19 of Electron? 11 - version currently used by meteor-desktop (Node 12) 14 - last version using Node 14, same as Meteor 2.x's Node 17 - last version compatible with Spectron (spectron@19) (Node 16) 27 - latest version (Node 18)

It would be interesting to know if having Electron run a more recent version of Node than Meteor could be a problem. I suspect not, beyond users having to be careful which Node features they use in the main VS renderer processes, but I'm not sure.

As for the tests, I agree it'd be great to replace Spectron by something else. I experimented with (Selenium) WebDriver last month and I was able to make it work easily with Electron, so that's another good candidate. I don't have time to rewrite the whole test suite now unfortunately. I'd say the person who will should choose the framework. In the meantime, we could also use this PR to update to Electron 17 already (or 14, if the Node version is an issue), if that makes the Spectron tests work.

Indeed, I also wonder. Otherwise we have to stick to Electron 14 as the highest until Meteor 3.0 comes out. That would be bad.

I'd say the person who will should choose the framework.

Fair point. In the end that person needs to be comfortable with the testing framework. Otherwise no one will do it.

It's fine for me to use Electron 17 in this merge request if integration test pass. Will check that.

Thanks @mjcctech for immediately pushing that. Let's hope it will work, that would be great progress.

mjcctech · 2023-11-15T07:27:38Z

skeleton/preload.js

-
-    global.Desktop = Desktop;
+    // exposeInMainWorld support only plain object with methods declared on first level
+    contextBridge.exposeInMainWorld('Desktop', Desktop.asJSON());


exposing electron module webFrame cause Electron contextBridge recursion depth exceeded. Nested objects deeper than 1000 are not supported error. Now integration test pass because I fake this module in line 296 electron: { webFrame: { getZoomFactor: () => 1 } },

I don't know how to overcome this issue. I'm not using this functionality.

Well, the tests are green, so that is a good first step. The question is how much else is needed from the electron object there, or if we can clone the object with limited depth to fix the original issue.

Well, the tests are green, so that is a good first step.

Jan (@StorytellerCZ ) which other steps are needed to commit this to a RC branch so we can test it out?

I will do an rc release for 3.2, if I get some time in the next few days.

If not, then ping me on Monday here:
https://www.youtube.com/watch?v=8s65DpKSVeQ

@StorytellerCZ you are going to solve issue with exposing electron modules, right? Otherwise I propose to remove this functionality from meteor-desktop, make breaking change and put in release notes information about it.

I will do an rc release for 3.2, if I get some time in the next few days.

Hi @StorytellerCZ ,

can you please do a RC release?

Just got a notification from Snyk that there’s a vulnerability for Electron 11.5.0: https://security.snyk.io/vuln/SNYK-JS-ELECTRON-6097142

Thank you.

Published communitypackages:[email protected].
Published communitypackages:[email protected].
@meteor-community/[email protected] under the preview tag.

a4xrbj1 · 2023-12-06T08:22:09Z

Upgraded to the new RC, started my Electron app (without desktopHCP) and I’m getting the following error:

INFO  electronApp:  transpiling and uglifying
ERROR  electronApp:  error while transpiling or minifying:  TypeError: api.caller is not a function
    at builder (/Users/andreaswest/Documents/workspace/frontend/node_modules/@babel/preset-env/src/index.ts:390:13)
    at /Users/andreaswest/Documents/workspace/frontend/node_modules/@babel/helper-plugin-utils/src/index.ts:62:12
    at ElectronApp.transpileAndMinify (/Users/andreaswest/Documents/workspace/frontend/node_modules/@meteor-community/meteor-desktop/lib/electronApp.js:854:33)
    at ElectronApp.build (/Users/andreaswest/Documents/workspace/frontend/node_modules/@meteor-community/meteor-desktop/lib/electronApp.js:451:24)
    at MeteorDesktop.run (/Users/andreaswest/Documents/workspace/frontend/node_modules/@meteor-community/meteor-desktop/lib/index.js:123:9)
INFO  electronApp:  packing .desktop to asar

It continues with building the app but then I get this fatal error:

debug: [main] desktop loaded
info: [localServer] will serve from:  /Users/andreaswest/Documents/workspace/frontend/.meteor/desktop-build/meteor.asar
(node:13251) electron: The default of contextIsolation is deprecated and will be changing from false to true in a future release of Electron.  See https://github.com/electron/electron/issues/23506 for more information
info: [localServer] assigned port 57207
error: [main]  message=this.webContents.setWindowOpenHandler is not a function, stack=TypeError: this.webContents.setWindowOpenHandler is not a function
    at App.onServerReady (/Users/andreaswest/Documents/workspace/frontend/.meteor/desktop-build/app/app.js:620:26)
    at Server.<anonymous> (/Users/andreaswest/Documents/workspace/frontend/.meteor/desktop-build/app/modules/localServer.js:585:26)
    at Server.emit (events.js:315:20)
    at emitListeningNT (net.js:1347:10)
    at processTicksAndRejections (internal/process/task_queues.js:83:21)
error: [main] error while emitting 'unhandledException' event: TypeError: Cannot read property 'close' of undefined

This is the code block (in app.js) that is causing the error:

this.webContents.setWindowOpenHandler((details) => {
            const { url } = details;
            const overrideOptions = url.startsWith('meteor://') ? {
                overrideBrowserWindowOptions: {
                    webPreferences: windowSettings.webPreferences,
                    parent: this.window
                }
            } : {};
            const result = {
                action: 'allow',
                ...overrideOptions
            };
            this.emit('childWindow', result, details);
            return result;
        });

I’m not sure if they are related to each other and how I can fix the second (breaking) one. This doesn’t look like code that we’ve added, not sure if anyone else can replicate this error.

Any advise is appreciated and thanks in advance!

mjcctech · 2023-12-06T08:40:04Z

@a4xrbj1 have you updated electron version? setWindowOpenHandler replace new-window event in electron 13. https://www.electronjs.org/docs/latest/breaking-changes#planned-breaking-api-changes-130

a4xrbj1 · 2023-12-06T08:57:54Z

@a4xrbj1 have you updated electron version? setWindowOpenHandler replace new-window event in electron 13. https://www.electronjs.org/docs/latest/breaking-changes#planned-breaking-api-changes-130

Nope, haven’t. I first wanted to see if upgrading Meteor-desktop only would lead to any problems - which it did. So I can proceed with upgrading Electron to version.

Just to confirm, did you upgrade to Electron version 17.4.11 (recommended version) or which exact version did you run?

I also got a warning to upgrade electron-builder from version 23.6.0 to 24.6.4 - which do that as a final step.

a4xrbj1 · 2023-12-06T09:48:49Z

Can confirm everything works like it should. I did update these packages:

"@meteor-community/meteor-desktop": "3.2.0-rc.0",

"electron": "17.4.11",

"electron-builder": "24.6.4",

@StorytellerCZ - I think you can make it a proper version.

My babel problem above still exists but I think it’s not a meteor-desktop problem.

StorytellerCZ · 2023-12-07T08:42:12Z

OK, we should mention the need to update the electron package manually in the changelog.

mjcctech added 2 commits November 13, 2023 13:46

ci: update to electron 19

2e60c63

test: fix test

6959d48

test: update spectron

fc15ad7

test: downgrade electron to 17

07dedde

chore: fix exposing Desktop.electron

b8c0bfe

mjcctech force-pushed the MX002-8311.1 branch from be9fcbb to b8c0bfe Compare November 14, 2023 14:48

mjcctech added 4 commits November 14, 2023 18:55

chore: other asJSON implementation

de8679b

chore: expose early

f358858

chore: one more asJSON implementation

82ab42a

chore: fake electron plugins

19b6b8c

mjcctech commented Nov 15, 2023

View reviewed changes

mjcctech changed the title ~~ci: update to electron 19~~ ci: update to electron 17 Nov 15, 2023

StorytellerCZ merged commit 51377f2 into Meteor-Community-Packages:master Dec 7, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: update to electron 17 #36

ci: update to electron 17 #36

mjcctech commented Nov 13, 2023 •

edited

Loading

a4xrbj1 commented Nov 13, 2023

mjcctech commented Nov 13, 2023

a4xrbj1 commented Nov 13, 2023 •

edited

Loading

StorytellerCZ commented Nov 13, 2023

a4xrbj1 commented Nov 13, 2023 •

edited

Loading

dd137 commented Nov 14, 2023 •

edited

Loading

mjcctech commented Nov 14, 2023 •

edited

Loading

a4xrbj1 commented Nov 14, 2023

mjcctech Nov 15, 2023 •

edited

Loading

StorytellerCZ Nov 15, 2023

a4xrbj1 Nov 15, 2023

StorytellerCZ Nov 15, 2023

mjcctech Nov 15, 2023

a4xrbj1 Dec 4, 2023 •

edited

Loading

StorytellerCZ Dec 4, 2023

a4xrbj1 commented Dec 6, 2023

mjcctech commented Dec 6, 2023

a4xrbj1 commented Dec 6, 2023

a4xrbj1 commented Dec 6, 2023

StorytellerCZ commented Dec 7, 2023

ci: update to electron 17 #36

ci: update to electron 17 #36

Conversation

mjcctech commented Nov 13, 2023 • edited Loading

What

Why

a4xrbj1 commented Nov 13, 2023

mjcctech commented Nov 13, 2023

a4xrbj1 commented Nov 13, 2023 • edited Loading

StorytellerCZ commented Nov 13, 2023

a4xrbj1 commented Nov 13, 2023 • edited Loading

dd137 commented Nov 14, 2023 • edited Loading

mjcctech commented Nov 14, 2023 • edited Loading

a4xrbj1 commented Nov 14, 2023

mjcctech Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

StorytellerCZ Nov 15, 2023

Choose a reason for hiding this comment

a4xrbj1 Nov 15, 2023

Choose a reason for hiding this comment

StorytellerCZ Nov 15, 2023

Choose a reason for hiding this comment

mjcctech Nov 15, 2023

Choose a reason for hiding this comment

a4xrbj1 Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

StorytellerCZ Dec 4, 2023

Choose a reason for hiding this comment

a4xrbj1 commented Dec 6, 2023

mjcctech commented Dec 6, 2023

a4xrbj1 commented Dec 6, 2023

a4xrbj1 commented Dec 6, 2023

StorytellerCZ commented Dec 7, 2023

mjcctech commented Nov 13, 2023 •

edited

Loading

a4xrbj1 commented Nov 13, 2023 •

edited

Loading

a4xrbj1 commented Nov 13, 2023 •

edited

Loading

dd137 commented Nov 14, 2023 •

edited

Loading

mjcctech commented Nov 14, 2023 •

edited

Loading

mjcctech Nov 15, 2023 •

edited

Loading

a4xrbj1 Dec 4, 2023 •

edited

Loading