[electron] Update Theia to use Electron 3.x and Node 10.x #4750

thegecko · 2019-03-28T14:13:37Z

This fixes #2009, #3729

The PR brings in changes to update Theia to Electron 3, which specifically uses:

Node 10.2.0
Chrome 66
V8 6.6

Please discuss impact concerns below.

It also updates the minimum node version used by Theia to v10.x.x

To do:

Await 0.5 release
Add update to CHANGELOG mentioning breaking changes
Update docs to use Node 10
~~Create CQ for wdio-mocha-framework: 0.6.4~~ (dev dependency]
~~Create CQ for vscode-nsfw: 1.1.2~~ (nsfw 1.1.2 just merged into master)
Create CQ for Electron: 3.17 [@thegecko]
Fix tests
Await CQ for Electron: 3.17: CQ 19441

cc @debovema

akosyakov · 2019-03-28T14:32:09Z

Generally, we (TypeFox) are for it, but could we hold it for one week till next Saturday? We have some releases depending on next, switching to node 10 could cause problems for them. After that we can merge it and land in April 0.6.0 release. bwt please update CHANGELOG with 0.6.0 breaking change, since we don't support node 8

Not sure how going for node 10 affects other products. @benoitf @caseyflynn-google

thegecko · 2019-03-28T14:45:08Z

but could we hold it for one week till next Saturday?

Absolutely

marcdumais-work · 2019-03-28T15:02:05Z

It also updates the minimum node version used by Theia to v10.x.x

That's a very important detail; maybe squeeze a mention in the PR's label to that it's clear and people will know this is tentatively coming soon?

marcdumais-work · 2019-03-28T17:43:58Z

Create CQ for wdio-mocha-framework: 0.6.4

This one is not necessary: it's a test/build dependency, already covered by a CQ, for "version: 0.5.9 and later versions"

thegecko · 2019-03-29T09:33:30Z

CQ created: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=19441

.travis.yml

thegecko · 2019-04-01T17:32:30Z

packages/core/src/browser/tree/tree.spec.ts

@@ -174,8 +174,7 @@ describe('Tree', () => {
      const expectedRefreshedNodes = new Set([
        retrieveNode<CompositeTreeNode>('1'),
        retrieveNode<CompositeTreeNode>('1.1'),
-        retrieveNode<CompositeTreeNode>('1.2'),
-        retrieveNode<CompositeTreeNode>('1.2.1')]);


There is a race condition here where not all nodes are deleted before the refresh() promise returns.

Had a hard time with this one but why this test was OK before migration to node 10 ?

I think the race simply gets further in different Node versions :/

packages/search-in-workspace/src/node/ripgrep-search-in-workspace-server.slow-spec.ts

thegecko · 2019-04-01T18:55:50Z

This is ready for review, pending the CQ being approved.

Some guidance/help on the commented test would be really useful.

cc @marcdumais-work , @akosyakov

paul-marechal · 2019-04-02T15:28:06Z

https://nodejs.org/docs/latest-v10.x/api/child_process.html#child_process_subprocess_connected

Maybe Node's child_process module behaves differently now. From my tests, in the RawProcess class the process object is actually created, but its .connected value is false, and the stdio pipes are left undefined on said object. Maybe just add a test for that, and break out by throwing a synchronous error (so that the RawProcess streams are set to something not undefined?

Signed-off-by: Rob Moran <rob.moran@arm.com>

thegecko · 2019-04-02T21:28:54Z

Thanks @marechal-p . It transpires the process pipes are undefined in Node 10 when using a non-executable command.

thegecko · 2019-04-02T22:11:03Z

Tests fixed and PR ready for final review.

akosyakov · 2019-04-03T07:03:42Z

@thegecko we will try on Monday, busy with internal releases

akosyakov · 2019-04-08T05:34:03Z

packages/preview/src/browser/markdown/markdown-preview-handler.ts

@@ -106,7 +106,7 @@ export class MarkdownPreviewHandler implements PreviewHandler {
        if (!elementToReveal) {
            return;
        }
-        elementToReveal.scrollIntoView({ behavior: 'instant' });
+        elementToReveal.scrollIntoView();


@AlexTugarev What are consequences of this change? Could you review the preview extension in this PR?

I wonder why it's affected. It's not part of Node.js, but DOM api.

@akosyakov, instant scroll behavior was experimental, apparently they removed it leaving smooth and auto. this change shouldn't make any difference.

fyi, instant was in place just to make sure it isn't using smooth by default, which would be bogus as it's not an atomic operation.

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

akosyakov · 2019-04-08T05:55:14Z

The browser version works nicely in Gitpod. Checking Electron locally. @kittaakos could you check as well?

akosyakov · 2019-04-08T05:57:46Z

@thegecko I've pushed one commit to update Gitpod. fix #4818

akosyakov

It works nicely for me in local Electron as well. Tested that typescript language support is there and Node.js debugging works. The only issue i've stumbled upon that yarn start picks up wrong Node.js version, so has to start with npx theia start.

akosyakov · 2019-04-08T07:05:10Z

Does any have objections to merge it or concerns regarding what can go wrong? It's time for testing.

vince-fugnitto · 2019-04-08T11:46:57Z

@thegecko @akosyakov I tried running the example-electron application and was unsuccessful.
I ran both yarn start and npx theia start but no luck :(

I've tried both on macOS and Ubuntu using Node v10.15.3. Perhaps I am doing something wrong.

(electron-3 ✓) npx theia start
Starting the master backend process with 5000 (ms) timeout.
Starting server worker...
Server worker has been started. [ID: 1 | PID: 6979]
/home/evinfug/workspace/electron-theia/theia/node_modules/electron/dist/electron: symbol lookup error: /home/evinfug/workspace/electron-theia/theia/node_modules/nsfw/build/Release/nsfw.node: undefined symbol: _ZN2v816FunctionTemplate3NewEPNS_7IsolateEPFvRKNS_20FunctionCallbackInfoINS_5ValueEEEENS_5LocalIS4_EENSA_INS_9SignatureEEEiNS_19ConstructorBehaviorENS_14SideEffectTypeE
Server worker has been disconnected. [ID: 1 | PID: 6979]
Server worker failed to start within 5000 milliseconds.
Pass a greater value as '--startup-timeout' option to increase the timeout or a negative to disable.
/home/evinfug/workspace/electron-theia/theia/node_modules/electron/dist/electron /home/evinfug/workspace/electron-theia/theia/examples/electron/src-gen/frontend/electron-main.js: symbol lookup error: /home/evinfug/workspace/electron-theia/theia/node_modules/nsfw/build/Release/nsfw.node: undefined symbol: _ZN2v816FunctionTemplate3NewEPNS_7IsolateEPFvRKNS_20FunctionCallbackInfoINS_5ValueEEEENS_5LocalIS4_EENSA_INS_9SignatureEEEiNS_19ConstructorBehaviorENS_14SideEffectTypeE

thegecko · 2019-04-08T11:49:02Z

@vince-fugnitto did you run yarn rebuild:electron first?

vince-fugnitto · 2019-04-08T11:57:18Z

@vince-fugnitto did you run yarn rebuild:electron first?

Thank you @thegecko, forgot about that!

kittaakos · 2019-04-08T12:59:48Z

.gitpod.yml

- init: yarn install
-  command: cd examples/browser && yarn start --hostname 0.0.0.0 ../..
+- init: nvm install 10 && nvm use 10 && yarn
+  command: yarn --cwd examples/browser start ../..


This will eventually fix #4818 too but you have to restore --hostname 0.0.0.0, don't you?

Gitpod is capable to auto expose localhost now, providing hostname is redundant.

kittaakos · 2019-04-08T13:01:21Z

.gitpod.yml

@@ -1,8 +1,8 @@
 ports:
 - port: 3000
 tasks:
- init: yarn install
-  command: cd examples/browser && yarn start --hostname 0.0.0.0 ../..
+- init: nvm install 10 && nvm use 10 && yarn


Why do not we install 10.15.3 if the documentation says: "Preferably, use version 10.15.3"

what issues do you see with using latest node 10? before we were using latest node 8, seemed to work just fine

it's the same in travis config btw

I just do not understand why do we recommend a version in the documentation but do not stick to it in the gitpod script.

Electron ships with Node 10.15.3, there is a slight danger you could introduce incompatibilities if you build using a newer version, however I wouldn't want to restrict people to a specific release otherwise.

This was the same with Electron 2, you can't override the node version it uses.

IMO, the current set up gives the user the right amount of flexibility.

kittaakos · 2019-04-08T13:24:22Z

I can see the followings logged in the browser console after starting the electron application from this branch. Does anyone know where do they come from?

internal/process/warning.js:18 (node:5213) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.
writeOut @ internal/process/warning.js:18

/Users/akos.kitta/git/theia/node_modules/electron/dist/Electron.app/Contents/Resources/electron.asar/renderer/security-warnings.js:184 Electron Security Warning (Insecure Content-Security-Policy) This renderer process has either no Content Security
      Policy set or a policy with "unsafe-eval" enabled. This exposes users of
      this app to unnecessary security risks.

kittaakos

I have tried it locally, checked the TS-LS; it worked.

I have seen this when I closed the browser windows:

Attempting to call a function in a renderer window that has been closed or released.
Function provided here: 41.bundle.js:547:27
root INFO Stopped watching the git repository: file:///Users/akos.kitta/git/theia
Attempting to call a function in a renderer window that has been closed or released.
Function provided here: 41.bundle.js:547:27
Attempting to call a function in a renderer window that has been closed or released.
Function provided here: 41.bundle.js:547:27

thegecko · 2019-04-08T14:27:23Z

I have seen this when I closed the browser windows

I believe this also exists on master.

thegecko · 2019-04-08T14:28:00Z

@kittaakos I've created a separate issue to remove the usage of Buffer(): #4840

thegecko · 2019-04-08T14:32:37Z

All concerns seem to be dealt with, I'm merging this.

akosyakov · 2019-04-09T07:47:53Z

We should update next images in theia-apps: https://github.com/theia-ide/theia-apps/tree/master/theia-docker We've overlooked it and broke all of them with this PR!

thegecko · 2019-04-09T07:55:17Z

Should be a simple change to use node 10 alpine. I've created an issue, although I can't modify the labels.

akosyakov · 2019-04-09T08:20:56Z

@thegecko the time consuming part is to test that they build and start properly, and that a change does not affect latest images (using 0.5.0)

thegecko requested review from svenefftinge, kittaakos, akosyakov, paul-marechal and marcdumais-work March 28, 2019 14:13

thegecko changed the title ~~[electron] Update Theia to use Electron 3~~ [electron] Update Theia to use Electron 3.x and Node 10.x Mar 28, 2019

thegecko force-pushed the electron-3 branch from 4ba0997 to 4085f2d Compare March 28, 2019 16:02

debovema mentioned this pull request Mar 28, 2019

Electron 3 node 10 #4753

Closed

thegecko force-pushed the electron-3 branch 4 times, most recently from d7f4d6c to 116625e Compare April 1, 2019 15:20

akosyakov reviewed Apr 1, 2019

View reviewed changes

.travis.yml Show resolved Hide resolved

thegecko force-pushed the electron-3 branch from 116625e to 8add844 Compare April 1, 2019 16:09

thegecko commented Apr 1, 2019

View reviewed changes

thegecko force-pushed the electron-3 branch from 8add844 to 2aa851c Compare April 1, 2019 17:33

thegecko commented Apr 1, 2019

View reviewed changes

packages/search-in-workspace/src/node/ripgrep-search-in-workspace-server.slow-spec.ts Outdated Show resolved Hide resolved

thegecko mentioned this pull request Apr 1, 2019

Electron 3 rebased and updated tests #4748

Closed

thegecko force-pushed the electron-3 branch from 2aa851c to 10c0f5d Compare April 2, 2019 20:05

Update electron and node packages

2a826de

Signed-off-by: Rob Moran <rob.moran@arm.com>

thegecko force-pushed the electron-3 branch from 10c0f5d to 2a826de Compare April 2, 2019 21:25

akosyakov reviewed Apr 8, 2019

View reviewed changes

update gitpod to use node 10

13063eb

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

akosyakov mentioned this pull request Apr 8, 2019

GH-2009: Updated the dependencies to support Node.js 10+. #4556

Closed

akosyakov approved these changes Apr 8, 2019

View reviewed changes

kittaakos reviewed Apr 8, 2019

View reviewed changes

kittaakos approved these changes Apr 8, 2019

View reviewed changes

thegecko mentioned this pull request Apr 8, 2019

[node] Remove deprecated uses of Buffer() #4840

Closed

thegecko merged commit 7d2959f into master Apr 8, 2019

thegecko deleted the electron-3 branch April 8, 2019 14:32

kittaakos mentioned this pull request Apr 8, 2019

[gitpod] The Gitpod init script should not change my working directory #4818

Closed

thegecko mentioned this pull request Apr 9, 2019

[node] Update all docker images to use node 10 theia-ide/theia-apps#161

Closed

evidolob mentioned this pull request Apr 9, 2019

Use node 10.2.1 for che-theia images eclipse-che/che-theia#167

Merged

[electron] Update Theia to use Electron 3.x and Node 10.x #4750

[electron] Update Theia to use Electron 3.x and Node 10.x #4750

Conversation

thegecko commented Mar 28, 2019 • edited

akosyakov commented Mar 28, 2019

thegecko commented Mar 28, 2019

marcdumais-work commented Mar 28, 2019

marcdumais-work commented Mar 28, 2019

thegecko commented Mar 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thegecko commented Apr 1, 2019

paul-marechal commented Apr 2, 2019 • edited

thegecko commented Apr 2, 2019

thegecko commented Apr 2, 2019

akosyakov commented Apr 3, 2019

akosyakov Apr 8, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov commented Apr 8, 2019

akosyakov commented Apr 8, 2019

akosyakov left a comment

Choose a reason for hiding this comment

akosyakov commented Apr 8, 2019 • edited

vince-fugnitto commented Apr 8, 2019

thegecko commented Apr 8, 2019

vince-fugnitto commented Apr 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kittaakos commented Apr 8, 2019

kittaakos left a comment

Choose a reason for hiding this comment

thegecko commented Apr 8, 2019

thegecko commented Apr 8, 2019 • edited

thegecko commented Apr 8, 2019

akosyakov commented Apr 9, 2019

thegecko commented Apr 9, 2019 • edited

akosyakov commented Apr 9, 2019

thegecko commented Mar 28, 2019 •

edited

paul-marechal commented Apr 2, 2019 •

edited

akosyakov Apr 8, 2019 •

edited

akosyakov commented Apr 8, 2019 •

edited

thegecko commented Apr 8, 2019 •

edited

thegecko commented Apr 9, 2019 •

edited