Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[electron] Investigate whether we can bump up to a newer version #3693

Closed
kittaakos opened this issue Nov 30, 2018 · 14 comments
Closed

[electron] Investigate whether we can bump up to a newer version #3693

kittaakos opened this issue Nov 30, 2018 · 14 comments
Assignees
Labels
electron issues related to the electron target

Comments

@kittaakos
Copy link
Contributor

The problem in a nutshell:
With the current TS version, we can do Array.values(): IterableIterator<T>;. It is from /typescript/lib/lib.es2015.iterable.d.ts. But in electron, at runtime, we still have Node.js 8.x. So it will be a runtime error.

@kittaakos kittaakos added the electron issues related to the electron target label Nov 30, 2018
@thegecko thegecko self-assigned this Nov 30, 2018
@thegecko thegecko changed the title [electron] Investigate whether we can bum up to a newer version [electron] Investigate whether we can bump up to a newer version Dec 1, 2018
@thegecko
Copy link
Member

thegecko commented Dec 1, 2018

So far I have electron upgraded to version 2.0.14 which is still using node 8.9.x.

It's unbelievably slow to debug however!

Is the purpose of the task to get to node 10.x as well, and/or can this upgrade be done in stages?

@thegecko
Copy link
Member

thegecko commented Dec 1, 2018

Does anyone know why Theia seems to be using a modified copy of nsfw:

https://www.npmjs.com/package/vscode-nsfw

The npm page links it back to the original source which makes it look suspicious.

@thegecko
Copy link
Member

thegecko commented Dec 1, 2018

@kittaakos re: the original issue above, as Theia targets ES5, I was under the impression tsc poly-fills newer features to work correctly. If this isn't the case, I'd be concerned about the general browser compatibility of Theia.

@kittaakos
Copy link
Contributor Author

So far I have electron upgraded to version 2.0.14 which is still using node 8.9.x.

👍

It's unbelievably slow to debug however!

Debugging the electron application was always extremely slow on my local environment.

Is the purpose of the task to get to node 10.x as well, and/or can this upgrade be done in stages?

No. +1 for tackling the Node.js version update in a separate PR.

The npm page links it back to the original source which makes it look suspicious.

The packge.json was forked as well.

@akosyakov
Copy link
Member

@thegecko nsfw was not supported by an original author for long time, and vscode team forked to fix some crucial bugs and make it usable for Electron 3 as far as I know. We use their fork.

@akosyakov
Copy link
Member

If this isn't the case, I'd be concerned about the general browser compatibility of Theia.

es5 is well supported by latest browsers already, the issue is only with electron

I was under the impression tsc poly-fills newer features to work correctly.

I believe that generally there is no such thing like tsc poly-fills for runtime features, tsc takes care about syntax compatibility. We could look 3rd party array polyfills, true. But then the latest stable electron is 3.0.10 with node 10.2.0. Could we switch to it?

@kittaakos
Copy link
Contributor Author

But then the latest stable electron is 3.0.10 with node 10.2.0.

I would not force Node.js 10x. VS Code does not do it either:
https://github.com/Microsoft/vscode/blob/253a4f6d17dfadd76c1bff0fcc429b8c3274e66d/build/npm/preinstall.js#L10-L13

@akosyakov
Copy link
Member

I don't mean forcing. Our baseline still will be the latest node 8 for backward compatibility. But if we use Electron 3, it already comes bundled with node 10.2.0. We cannot change it, I hope it would be fine and node 10 is compatible with node 8.

@thegecko
Copy link
Member

thegecko commented Dec 3, 2018

I continued looking into this yesterday and have Theia working with electron 3.
I did this by re-creating the yarn lock file, which isn't satisfactory as it has upgraded most packages. I'd like to work out what is needed and upgrade individual packages instead before creating a PR.

I did need to move everything to node 10 however as building things such as vscode-nsfw required the same version of node as is bundled with electron.

I'll try to get it working with node 8, but I'm not sure it's possible. We may need to decide to move to node 10 (for electron at least) in order to make this work.

@akosyakov
Copy link
Member

@thegecko thanks for checking, I've added it to tomorrow agenda to discuss with what we want to stick

@thegecko
Copy link
Member

thegecko commented Dec 4, 2018

Just got electron 3 built with node 8.x :)

Had to remove the Java plugin to get it working, though.

Will node 8.x be enough to use the features mentioned in the OP however?

@kittaakos
Copy link
Contributor Author

@thegecko, I am collecting the decision we made during the tcon, please add/update if needed:

  • We are targeting electron 2.x latest. So we get some security updates and hopefully, some new features will be supported in Chrome.
  • We monitor VS Code, once they move to electron 3.x, we try to update too.
  • We try to remove the problematic Array.values().
  • We stick to Node.js 8.x.
  • Pipe dream: find a linter rule that notifies us if we use a language feature that is not yet supported.

@thegecko
Copy link
Member

thegecko commented Dec 4, 2018

@kittaakos, that sounds about right.

I've created #3728, #3729 and #3730 to track the outcomes.

I think this issue can be closed in favour of them?

@kittaakos
Copy link
Contributor Author

I am closing this. Please reopen if required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
electron issues related to the electron target
Projects
None yet
Development

No branches or pull requests

3 participants