tsconfig: transpile to es6 #6060

paul-marechal · 2019-08-28T22:32:31Z

What it does

Transpile from es5 to es6 to accomodate libraries written in es6.

Reason is that es6 is backward compatible with es5 but it is not forward
compatible.

Closes #5902.

How to test

To be defined? yarn test should terminate successfully.

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

Transpile from es5 to es6 to accomodate libraries written in es6. Reason is that es6 is backward compatible with es5 but it is not forward compatible. Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>

akosyakov · 2019-08-29T03:17:54Z

To be defined? yarn test should terminate successfully.

It is not enough, we should use electron and browser apps and check that such constructions like instanceof are supported. As far as I know it probably won't work for phosphor js yet: phosphorjs/phosphor#413 i.e for any es5 code from dependencies which we extend.

paul-marechal · 2019-08-29T14:27:01Z

i.e for any es5 code from dependencies which we extend.

Why?

I just launched the browser and electron application, I was unable to find regressions UI-wise.

Just tried on the TypeScript playground again, and ES5 classes seem to be extendable from ES6 code: playground setup

paul-marechal · 2019-08-29T17:31:29Z

To be more clear, my claims are:

I wasn't able to find anything broken in this PR (but I am asking for help finding issues).
ES6 classes can extend ES5 classes, and class-specific operations still work (i.e. instanceof).
Only issue I had was related to the JsonRpcProxyFactory, since it relied on a for..in to list all methods (in old ES5), this seems to be changed in ES6: we have to be more clever in the lookup in order to fetch everything. I noticed the regression from the tests. I already took care of it in this PR.

paul-marechal · 2019-08-29T23:01:29Z

Oops, the core team is a bit overkill to ask on a PR, or is it intended that it would ping 21 people?

akosyakov · 2019-08-30T04:33:44Z

Oops, the core team is a bit overkill to ask on a PR, or is it intended that it would ping 21 people?

Why not? All these people are Theia committers, i.e. responsible for helping to maintaining Theia. If someone just wants to contribute they don't need write access, other committers can review and help to land their work.

akosyakov · 2019-08-30T04:56:16Z

packages/java/src/browser/java-client-contribution.ts

@@ -80,7 +80,7 @@ export class JavaClientContribution extends BaseLanguageClientContribution {
    protected onReady(languageClient: ILanguageClient): void {
        languageClient.onNotification(ActionableNotification.type, this.showActionableMessage.bind(this));
        languageClient.onNotification(StatusNotification.type, this.showStatusMessage.bind(this));
-        languageClient.onRequest(ExecuteClientCommand.type, params => this.commandService.executeCommand(params.command, ...params.arguments));
+        languageClient.onRequest(ExecuteClientCommand.type, params => this.commandService.executeCommand(params.command, ...params.arguments || []));


Why do we need to change it? Does it mean a breaking change for all JSON-RPC using varargs? Could we avoid it?

Switching compilation target to ES6 made TypeScript detect that params.arguments can be undefined meaning that it could not be unwrapped. It wouldn't let me build.

It is breaking if we try to unwrap undefined values in other places.

akosyakov · 2019-08-30T04:57:07Z

Let's have a look at it when Monaco upgrade is finished.

akosyakov · 2019-08-30T05:17:51Z

@AlexTugarev Do you remember how to reproduce inversify/InversifyJS#928 in Theia?

AlexTugarev · 2019-08-30T05:58:33Z

@akosyakov there was at least one case of an injection with bogus constructor arguments as described in the issue. the included code snipped should tell us if it was fixed in the meanwhile. the execution should produce the same output for es5 and es6.

I guess 74e5ac7#diff-7bcf3cc870db868677d26d618e924394 is related.

akosyakov · 2019-08-30T10:13:59Z

@AlexTugarev It means that it was only worked around in this repo, but does not guarantee it won't break for any downstream projects which can use such bindings.

Another issue that it is going to cause rippling effect by forcing all extensions and products be recompiled to es6. cc @svenefftinge

If we do it we have to collect all possible pitfalls and prepare PRs for extensions under theia ide org. Maybe we discuss it first, seems to be like a lot of work for us and extenders. It is not a showstopper for Monaco migration right now and can be postponed.

paul-marechal · 2019-08-30T14:34:03Z

@AlexTugarev After trying out your code snippet on inversify/InversifyJS#928, it looks like inversify sets to undefined arguments following ones marked as @optional() when targeting es6:

    constructor(
        @inject(DepC) c: DepC,
        @inject(DepB) @optional() b: DepB = {b: 0},
        @inject(DepA) a: DepA,
        @inject(DepB) @optional() d: DepD = {d: 0},
        @inject(DepA) e: DepE,
        @inject(DepB) @optional() f: DepF = {f: 0},
    )

In this example, a and e will be undefined. This is definitely a bug, but if we put all optional parameters at the end, no more undefined values. Not saying this is not an issue, but it lessens the problem a bit (I find it odd to interleave optional parameters like this).

paul-marechal · 2019-08-30T14:34:46Z

It is not a showstopper for Monaco migration right now and can be postponed.

Ok

Another issue that it is going to cause rippling effect by forcing all extensions and products be recompiled to es6.

@akosyakov Indeed, since es5 extensions will likely break when importing updated es6 core components... Does that mean that we should systematically use babel to ensure everything is transpiled to the oldest ecma spec?

You also mentioned that babel was a no-go with the plugin extension, how so?

AlexTugarev · 2019-08-30T14:56:34Z

it looks like inversify sets to undefined arguments following ones marked as @optional() when targeting es6:

@marechal-p that would be a changed behavior then. what I reported back then was that even optionals a not injected properly in subclasses.

paul-marechal · 2019-08-30T15:15:35Z

what I reported back then was that even optionals a not injected properly in subclasses.

Ah indeed you are right, I only noticed the odd undefined values. After more testing, it is any parameter with a default value that breaks inversify, optional or not.

akosyakov · 2019-09-02T04:17:25Z

Indeed, since es5 extensions will likely break when importing updated es6 core components... Does that mean that we should systematically use babel to ensure everything is transpiled to the oldest ecma spec?

Not sure we want to run babel on the whole codebase, it could increase the build time significantly. In Monaco PR i only use babel to down compile several es6 dependencies.

You also mentioned that babel was a no-go with the plugin extension, how so?

backend does not go through webpack, unfortunately. But in monaco PR it is turned out to be not an issue to use es6 dependencies for the backend, i.e. we don't extend anything from them.

akosyakov · 2019-09-11T08:34:50Z

See another point which we should not overlook: #5902 (comment)

tsconfig: transpile to es6

bfe762a

Transpile from es5 to es6 to accomodate libraries written in es6. Reason is that es6 is backward compatible with es5 but it is not forward compatible. Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>

paul-marechal added the dependencies pull requests that update a dependency file label Aug 28, 2019

paul-marechal requested a review from akosyakov August 28, 2019 22:32

paul-marechal requested a review from a team as a code owner August 28, 2019 22:32

paul-marechal requested review from a team and removed request for a team August 29, 2019 22:58

akosyakov requested a review from a team August 30, 2019 04:33

akosyakov reviewed Aug 30, 2019

View reviewed changes

paul-marechal closed this Nov 22, 2019

kittaakos mentioned this pull request Dec 17, 2019

How to rebind FileSystemNode #6740

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tsconfig: transpile to es6 #6060

tsconfig: transpile to es6 #6060

paul-marechal commented Aug 28, 2019

akosyakov commented Aug 29, 2019

paul-marechal commented Aug 29, 2019 •

edited

Loading

paul-marechal commented Aug 29, 2019

paul-marechal commented Aug 29, 2019

akosyakov commented Aug 30, 2019 •

edited

Loading

akosyakov Aug 30, 2019

paul-marechal Aug 30, 2019 •

edited

Loading

akosyakov commented Aug 30, 2019

akosyakov commented Aug 30, 2019

AlexTugarev commented Aug 30, 2019

akosyakov commented Aug 30, 2019 •

edited

Loading

paul-marechal commented Aug 30, 2019 •

edited

Loading

paul-marechal commented Aug 30, 2019 •

edited

Loading

AlexTugarev commented Aug 30, 2019

paul-marechal commented Aug 30, 2019 •

edited

Loading

akosyakov commented Sep 2, 2019 •

edited

Loading

akosyakov commented Sep 11, 2019

tsconfig: transpile to es6 #6060

tsconfig: transpile to es6 #6060

Conversation

paul-marechal commented Aug 28, 2019

What it does

How to test

Review checklist

Reminder for reviewers

akosyakov commented Aug 29, 2019

paul-marechal commented Aug 29, 2019 • edited Loading

paul-marechal commented Aug 29, 2019

paul-marechal commented Aug 29, 2019

akosyakov commented Aug 30, 2019 • edited Loading

akosyakov Aug 30, 2019

Choose a reason for hiding this comment

paul-marechal Aug 30, 2019 • edited Loading

Choose a reason for hiding this comment

akosyakov commented Aug 30, 2019

akosyakov commented Aug 30, 2019

AlexTugarev commented Aug 30, 2019

akosyakov commented Aug 30, 2019 • edited Loading

paul-marechal commented Aug 30, 2019 • edited Loading

paul-marechal commented Aug 30, 2019 • edited Loading

AlexTugarev commented Aug 30, 2019

paul-marechal commented Aug 30, 2019 • edited Loading

akosyakov commented Sep 2, 2019 • edited Loading

akosyakov commented Sep 11, 2019

paul-marechal commented Aug 29, 2019 •

edited

Loading

akosyakov commented Aug 30, 2019 •

edited

Loading

paul-marechal Aug 30, 2019 •

edited

Loading

akosyakov commented Aug 30, 2019 •

edited

Loading

paul-marechal commented Aug 30, 2019 •

edited

Loading

paul-marechal commented Aug 30, 2019 •

edited

Loading

paul-marechal commented Aug 30, 2019 •

edited

Loading

akosyakov commented Sep 2, 2019 •

edited

Loading