ci: update to electron 17

[mjcctech]

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]

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]

@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]

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: https://github.com/Meteor-Community-Packages/meteor-desktop/issues/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]

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]

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]

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]

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]

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.

[a4xrbj1]

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]

@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]

@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]

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]

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