Node.js 12.x + Electron 9x #7968

kittaakos · 2020-06-04T17:18:46Z

What it does

Adds support for Node.js 12.x,
Aligns the recommended Node.js version with the one shipped with electron 9.x,
Pins the node-abi version, and update the @babel dependency to support Node.js 12.x,
Updates the electron version to 9.x, this PR adjusts the electron API used in Theia, and
Enables the Linux CI to run the tests on Node.js 10.x and 12.x.

Notes:

Electron v9.0.2
Chromium v83.0.4103.94
Node v12.14.1
v8 v8.3.110.9-electron.0

Closes #7349

How to test

You can build, start, and use Theia on both Node.js 10x and 12.x,
You can build, start, and use the electron app.

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

package.json

akosyakov · 2020-06-05T07:23:01Z

We should set Node.js 12.x as minimal in dependencies, typings, configs and docs that it is aligned between browser and electron targets.

paul-marechal

LGTM, the native menus on Windows also picked up my OS theme which looks much nicer!

dev-packages/application-manager/src/generator/frontend-generator.ts

packages/core/src/electron-browser/keyboard/electron-keyboard-layout-change-notifier.ts

dev-packages/application-manager/src/generator/frontend-generator.ts

thegecko · 2020-06-05T16:43:14Z

We should set Node.js 12.x as minimal in dependencies, typings, configs and docs that it is aligned between browser and electron targets.

A bit old now, but here's all the areas I updated when we went to Electron 4:

https://github.com/eclipse-theia/theia/pull/6307/files

Does this PR also update Node to v12?

marcdumais-work · 2020-06-05T19:14:32Z

We will need a CQ for at least the Electron part - this is a tricky component license-wise. I'll dig the two previous ones.

kittaakos · 2020-06-06T07:40:02Z

I added a preinstall script to notify the users,

akosyakov · 2020-06-08T07:03:56Z

I added a preinstall script to notify the users

Does not yarn takes care about it based on engine property? I am not concerned with contributors building Theia from this repo. But with adopters which are pulling from npm, they have to be aware that we can remove Node.js 10 support with any versions after 1.3.0.

kittaakos · 2020-06-08T07:44:05Z

Does not yarn takes care about it based on engine property?

It always fails, cannot be used for warnings.

But with adopters which are pulling from npm, they have to be aware that we can remove Node.js 10 support with any versions after 1.3.0.

I already expressed my confusion about why and who do you want to notify: #7962 (comment). Feel free to submit a change against this PR. You can also pin a message on Spectrum or where you think it has the greatest visibility.

kittaakos · 2020-06-08T07:47:05Z

Does this PR also update Node to v12?

Yes.

CHANGELOG.md

marcdumais-work · 2020-06-08T14:20:27Z

We will need a CQ for at least the Electron part - this is a tricky component license-wise. I'll dig the two previous ones.

CQ info: We have gone through this twice in the past, that I remember. We ended-up having two CQs each time: one for Electron and one for the corresponding bundled FFmpeg library:

Electron 3.1.7 and corresponding FFmpeg:
- https://dev.eclipse.org/ipzilla/show_bug.cgi?id=19441
- https://dev.eclipse.org/ipzilla/show_bug.cgi?id=19939
Electron 4.2.11 and corresponding FFmpeg:
- https://dev.eclipse.org/ipzilla/show_bug.cgi?id=20993
- https://dev.eclipse.org/ipzilla/show_bug.cgi?id=20994

The tricky part I think it to provide the attachment for FFmpeg, that reflects how the GPL content is excluded, when built for Electron. I found a way that I documented last time: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=20994#c10

We also need to validate that we are still excluding correctly the h264 codec, by downloading an alternate FFmpeg at build time (made available by the Electron project. We also test for this, failing the build if h264 is still present after replacement - need to validate this still works. (all this is part of @theia/electron)

In the spirit of spreading knowledge in the project, is there someone who would like to try registering these CQs for Electron 9x?
cc: @eclipse-theia/ip-help

akosyakov · 2020-06-09T13:45:22Z

@marcdumais-work Could you take care of it? I've learned today that code which I'm going to port from VS Code to support FS apis makes use of fs apis which are not supported by our current Node.js.

paul-marechal

Tested a few different actions/scenarios with the Electron example and all looks good.

kittaakos · 2020-07-15T06:44:59Z

Thanks a lot, @marechal-p 👍

I am going to leave this PR open for a day or so, so if anyone has concerns can object, then I am going to merge it.

akosyakov · 2020-07-15T08:30:21Z

@kittaakos one small thing: could you update gitpod dockerfile to use Node.js 12 too.

kittaakos · 2020-07-15T08:52:26Z

kittaakos one small thing: could you update gitpod dockerfile to use Node.js 12 too.

It's done. Please verify it. Thanks!

akosyakov · 2020-07-15T09:24:38Z

browser example works nicely, but electron does not start anymore:

$ theia start --plugins=local-dir:../../plugins ../.. --hostname=0.0.0.0
/workspace/theia/node_modules/electron/dist/electron: error while loading shared libraries: libgbm.so.1: cannot open shared object file: No such file or directory

It seems libgbm1 has to be added here: https://github.com/eclipse-theia/theia/blob/master/.gitpod.dockerfile#L11

to verify I run from the root:

yarn rebuild:electron
jwm & yarn --cwd examples/electron start ../.. --hostname=0.0.0.0

after that VNC session should be available on port 6080

kittaakos · 2020-07-15T09:34:13Z

It seems libgbm1 has to be added here: https://github.com/eclipse-theia/theia/blob/master/.gitpod.dockerfile#L11

to verify I run from the root:

Can you please take care of it? Gitpod is out of the context of an Electron update in the Theia repo.

If you do not have time, we can drop the Gitpod Docker-related changes and update it in a separate PR.

kittaakos · 2020-07-15T09:36:55Z

The test failure is unrelated:

root INFO   1 failing
root INFO   1) TypeScript
       editor.action.peekDefinition
         within an editor:

akosyakov · 2020-07-15T09:47:52Z

@kittaakos I will push a commit shortly. Keeping working Gitpod setup is as important as local.

kittaakos · 2020-07-15T09:50:54Z

I will push a commit shortly.

Sure. Feel free squashing and force pushing.

Keeping working Gitpod setup is as important as local.

👍 I agree, I just do not have the resources now to deal with the electron VNC issues for Gitpod.

- Updated the `@babel` dependencies to fix babel/babel#11216#issuecomment-634460665. - Pinned the `node-abi` version in the `resolutions` to support latest `electron`. - Use Node.js 12.14.1 for Gitpod. Closes #7349 Co-authored-by: Anton Kosyakov <[email protected]> Signed-off-by: Akos Kitta <[email protected]>

akosyakov · 2020-07-15T10:31:27Z

It seems to work now, but one has to pass --no-sandbox to Electron in Gitpod (or in docker container generally). I don't want to enable it always, so let's keep it as is for now.

marcdumais-work · 2020-07-15T21:46:47Z

It seems to work now, but one has to pass --no-sandbox to Electron in Gitpod (or in docker container generally). I don't want to enable it always, so let's keep it as is for now.

@akosyakov do you see a way forward? I have recently added a Gitpod config to one of our project, that's a collaboration between my department and a local university research project.

Only the Electron version of their example app can run on Gitpod, at the moment, because of a limitation in their extension. The browser version works but only locally. They can stay a little while on 1.3.0 I think, and avoid this issue momentarily.

marcdumais-work · 2020-07-15T21:49:15Z

cc: @tahini - see just above

marcdumais-work · 2020-07-15T22:26:04Z

I tested briefly on node 10 and 12, and electron. No problem found 👍

@kittaakos I assume we'll need to update electron-builder version and maybe config for the Electron packaging example in theia-apps?

kittaakos · 2020-07-16T06:32:10Z

I tested briefly on node 10 and 12, and electron. No problem found 👍

Great news!

@kittaakos I assume we'll need to update electron-builder version and maybe config for the Electron packaging example in theia-apps?

Why do you think we have to update the electron-builder, @marcdumais-work?

config for the Electron packaging example in theia-apps?

Did you mean the node restriction here?

kittaakos · 2020-07-16T06:33:09Z

Thank you all for the help with the reviews, verifications, and the CQs.

kittaakos · 2020-07-16T12:27:31Z

Why do you think we have to update the electron-builder, @marcdumais-work?

@marcdumais-work, I wanted to update the electron app in the theia-apps repo but, I've just noticed we use latest instead of next, so we cannot use Node.js 12.x for building the app or using electron 9.x unless we have another Theia release. Once we have the next release and if there is an issue with the electron app, please file a bug report and assign it to me. I am going to take care of it.

marcdumais-work · 2020-07-16T14:11:17Z

Why do you think we have to update the electron-builder, @marcdumais-work?

Since we're making quite a big Electron version jump with this update, it seemed probable that e.g. we might need to update to a newer electron-builder version. I could not easily test that, so I wondered what you thought.

Did you mean the node restriction here?

Not specifically, but good catch - probably at least that needs updating.

Thanks for this PR!

akosyakov · 2020-07-25T04:56:29Z

@akosyakov do you see a way forward? I have recently added a Gitpod config to one of our project, that's a collaboration between my department and a local university research project.

@marcdumais-work They can use --no-sandbox too. If it is so common, maybe we could add some env variable or something like that to automatically apply --no-sandbox in containers.

Fixes #106 A few little tweaks are necessary to accommodate the new Electron and node versions. ATM the Electron example app fails at runtime when running on Gitpod, because of a missing a dynamic library: /workspace/theia-trace-extension/node_modules/electron/dist/electron: error while loading shared libraries: libgbm.so.1: cannot We need to update the Gitpod config so the dockerfile used for this repoi will contain a new Ubuntu package, required for Electron 9. This config file originally came from the main Theia repo, and the updated version is the same. See LICENSE.gitpod.md for more details. Here's where the --no-sandbox idea came from - it permits the Electron version of the example app to start in Gitpod: eclipse-theia/theia#7968 (comment) Signed-off-by: Marc Dumais <[email protected]>

Fixes #106 A few little tweaks are necessary to accommodate the new Electron and node versions. ATM the Electron example app fails at runtime when running on Gitpod, because of a missing a dynamic library: /workspace/theia-trace-extension/node_modules/electron/dist/electron: error while loading shared libraries: libgbm.so.1: cannot We need to update the Gitpod config so the dockerfile used for this repoi will contain a new Ubuntu package, required for Electron 9. This config file originally came from the main Theia repo, and the updated version is the same. See LICENSE.gitpod.md for more details. Here's where the --no-sandbox idea came from - it permits the Electron version of the example app to start in Gitpod: eclipse-theia/theia#7968 (comment) Also, for the Electron version of the example application to work correctly in Gitpod (potentially in any container-based environment), it's required to use the "--ignore-gpu-blacklist" startup paramter. Signed-off-by: Marc Dumais <[email protected]>

kittaakos changed the base branch from master to GH-7349 June 4, 2020 17:22

kittaakos commented Jun 4, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

kittaakos force-pushed the electron-9x branch from b8072d4 to 5e1b19d Compare June 5, 2020 07:04

akosyakov added the electron issues related to the electron target label Jun 5, 2020

akosyakov requested review from thegecko and marcdumais-work June 5, 2020 07:19

kittaakos mentioned this pull request Jun 5, 2020

GH-7349: Support for Node.js 12.x #7962

Closed

1 task

kittaakos requested review from paul-marechal and akosyakov June 5, 2020 07:23

kittaakos marked this pull request as ready for review June 5, 2020 07:25

akosyakov added the dependencies pull requests that update a dependency file label Jun 5, 2020

paul-marechal reviewed Jun 5, 2020

View reviewed changes

kittaakos force-pushed the electron-9x branch from 5e1b19d to 0432864 Compare June 5, 2020 18:46

kittaakos changed the base branch from GH-7349 to master June 5, 2020 18:47

marcdumais-work added the CQ Required (deprecated) issues requiring a CQ (contributor questionnaire) label Jun 5, 2020

kittaakos force-pushed the electron-9x branch from 0432864 to 6c0efea Compare June 6, 2020 07:37

kittaakos force-pushed the electron-9x branch from 6c0efea to af5ddcf Compare June 8, 2020 07:35

kittaakos force-pushed the electron-9x branch from af5ddcf to 347837e Compare June 8, 2020 07:57

kittaakos changed the title ~~Electron 9x~~ Node.js 12.x + Electron 9x Jun 8, 2020

kittaakos requested a review from benoitf June 8, 2020 13:12

kittaakos commented Jun 8, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

paul-marechal approved these changes Jul 14, 2020

View reviewed changes

kittaakos force-pushed the electron-9x branch from b7621f0 to 094ffe0 Compare July 15, 2020 08:52

akosyakov force-pushed the electron-9x branch from 094ffe0 to fdd6045 Compare July 15, 2020 09:58

kittaakos merged commit 9afc819 into master Jul 16, 2020

kittaakos mentioned this pull request Jul 16, 2020

Cannot start electron using node 12 #8190

Closed

marcdumais-work mentioned this pull request Jul 30, 2020

update repo for Electron 9 / node 12 eclipse-cdt-cloud/theia-trace-extension#107

Merged

kittaakos deleted the electron-9x branch August 18, 2020 10:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node.js 12.x + Electron 9x #7968

Node.js 12.x + Electron 9x #7968

kittaakos commented Jun 4, 2020 •

edited

Loading

akosyakov commented Jun 5, 2020

paul-marechal left a comment

thegecko commented Jun 5, 2020

marcdumais-work commented Jun 5, 2020

kittaakos commented Jun 6, 2020

akosyakov commented Jun 8, 2020

kittaakos commented Jun 8, 2020

kittaakos commented Jun 8, 2020

marcdumais-work commented Jun 8, 2020

akosyakov commented Jun 9, 2020

paul-marechal left a comment

kittaakos commented Jul 15, 2020

akosyakov commented Jul 15, 2020

kittaakos commented Jul 15, 2020

akosyakov commented Jul 15, 2020

kittaakos commented Jul 15, 2020

kittaakos commented Jul 15, 2020

akosyakov commented Jul 15, 2020 •

edited

Loading

kittaakos commented Jul 15, 2020

akosyakov commented Jul 15, 2020

marcdumais-work commented Jul 15, 2020 •

edited

Loading

marcdumais-work commented Jul 15, 2020

marcdumais-work commented Jul 15, 2020

kittaakos commented Jul 16, 2020

kittaakos commented Jul 16, 2020

kittaakos commented Jul 16, 2020

marcdumais-work commented Jul 16, 2020

akosyakov commented Jul 25, 2020

Node.js 12.x + Electron 9x #7968

Node.js 12.x + Electron 9x #7968

Conversation

kittaakos commented Jun 4, 2020 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

akosyakov commented Jun 5, 2020

paul-marechal left a comment

Choose a reason for hiding this comment

thegecko commented Jun 5, 2020

marcdumais-work commented Jun 5, 2020

kittaakos commented Jun 6, 2020

akosyakov commented Jun 8, 2020

kittaakos commented Jun 8, 2020

kittaakos commented Jun 8, 2020

marcdumais-work commented Jun 8, 2020

akosyakov commented Jun 9, 2020

paul-marechal left a comment

Choose a reason for hiding this comment

kittaakos commented Jul 15, 2020

akosyakov commented Jul 15, 2020

kittaakos commented Jul 15, 2020

akosyakov commented Jul 15, 2020

kittaakos commented Jul 15, 2020

kittaakos commented Jul 15, 2020

akosyakov commented Jul 15, 2020 • edited Loading

kittaakos commented Jul 15, 2020

akosyakov commented Jul 15, 2020

marcdumais-work commented Jul 15, 2020 • edited Loading

marcdumais-work commented Jul 15, 2020

marcdumais-work commented Jul 15, 2020

kittaakos commented Jul 16, 2020

kittaakos commented Jul 16, 2020

kittaakos commented Jul 16, 2020

marcdumais-work commented Jul 16, 2020

akosyakov commented Jul 25, 2020

kittaakos commented Jun 4, 2020 •

edited

Loading

akosyakov commented Jul 15, 2020 •

edited

Loading

marcdumais-work commented Jul 15, 2020 •

edited

Loading