Compile down to es6

[João Moreno (joaomoreno)]

Could we compile our sources to ES6? This can bring many engineering advantages, especially around debugging our sources, and possibly perf improvements.

This branch now has a couple of errors.

Most of them are related to the fact that it doesn't seem to be possible to use async and TPromise with ES6 anymore:

Error: /Users/joao/Work/vscode/src/vs/code/electron-main/logUploader.ts(40,4): The return type of an async function or method must be the global Promise<T> type.

Matt Bierner (@mjbvz) Is this a real limitation, or can we get around it somehow, without converting all our promises?

Other errors lie in lib.ie11_safe_es6.d.ts:

[15:29:46] Error: /Users/joao/Work/vscode/src/typings/lib.ie11_safe_es6.d.ts(28,2): All declarations of 'prototype' must have identical modifiers.
[15:29:46] Error: /Users/joao/Work/vscode/src/typings/lib.ie11_safe_es6.d.ts(54,2): All declarations of 'prototype' must have identical modifiers.
[15:29:46] Error: /Users/joao/Work/vscode/src/typings/lib.ie11_safe_es6.d.ts(73,24): Type 'K' does not satisfy the constraint 'object'.
[15:29:46] Error: /Users/joao/Work/vscode/src/typings/lib.ie11_safe_es6.d.ts(75,11): Subsequent property declarations must have the same type.  Property 'prototype' must be of type 'WeakMap<object, any>', but here has type 'WeakMap<any, any>'.

There are a few others, but let's get these out of the way; they seem bigger.

cc @Microsoft/vscode

[Johannes Rieken (jrieken)]

Careful with that because I believe it leaks globals into the CU that we cannot use, e.g Symbol.iterator etc. We need to configure the compiler so that it emits ES6 but doesn't use ES6 globals...

[roblourens]

Why can't we use that, is it not in electron's Node?

[Johannes Rieken (jrieken)]

The monaco editor supports IE11

[João Moreno (joaomoreno)]

We must use a different compilation pass for the monaco editor. But I do see the problem that we might bring in references to unavailable stuff without realising it. Could we also solve that with some linting?

[Johannes Rieken (jrieken)]

We have src/tsconfig.monaco.json which is only the monaco editor. Problem is that tsserver can only use one project file so that you won't see compile errors in VS Code, e.g. when using Symbol.iterator in vs/base or vs/editor. Running tsc -p tsconifg.monaco.json takes around 15secs which I'd say is too much for a pre-commit hook. It does already run travis et al but with little respect...

[Matt Bierner (mjbvz)]

Ron Buckton (@rbuckton) / Andy (Andrewkraft) (@Andy-MS) Any ideas on using a custom promise type (TPromise) with async?

[Johannes Rieken (jrieken)]

ping Ron Buckton (@rbuckton) / Andy (Andrewkraft) (@Andy-MS)

[Johannes Rieken (jrieken)]

So, I believe the challenge is that our WinJS.Promise isn't using well known symbols (and it cannot because of the continued support of IE11) Therefore it isn't compatible with the native promise (as it is defined with es6). I have tried to define the libs we use so that it contains everything from es6 but not well known symbols and seems to help. However, it triggers different errors that I don't understand.

This is the tsconfig.json-file

{
	"compilerOptions": {
		"module": "amd",
		"moduleResolution": "classic",
		"noImplicitAny": false,
		"removeComments": false,
		"preserveConstEnums": true,
		"target": "es6",
		"lib": [
			"dom",
			"dom.iterable",
			"es2015.collection",
			"es2015.core",
			"es2015.generator",
			"es2015.iterable",
			"es2015.promise",
			"es2015.proxy",
			"es2015.reflect",
			"es2015.symbol",
			"webworker"
		],
		"sourceMap": false,
		"experimentalDecorators": true,
		"declaration": true,
		"noImplicitReturns": true,
		"noUnusedLocals": true,
		"noImplicitThis": true,
		"alwaysStrict": true,
		"baseUrl": ".",
		"outDir": "../out",
		"typeRoots": [
			"typings"
		]
	},
	"exclude": [
		"./typings/lib.ie11_safe_es6.d.ts",
		"./typings/es6-promise.d.ts"
	]
}

and there errors are generated

jrieken:~/Code/vscode$ tsc -p src/tsconfig.json
error TS2318: Cannot find global type 'Boolean'.
error TS2318: Cannot find global type 'Number'.
src/vs/base/browser/event.ts(22,92): error TS4047: Return type of call signature from exported interface has or is using private name 'MSManipulationEvent'.
src/vs/base/common/comparers.ts(13,47): error TS4078: Parameter 'collator' of exported function has or is using private name 'Intl'.
src/vs/base/common/date.ts(16,40): error TS4078: Parameter 'date' of exported function has or is using private name 'Date'.
src/vs/base/node/zip.ts(34,35): error TS4020: 'extends' clause of exported class 'ExtractError' has or is using private name 'Error'.
src/vs/editor/common/model/textModelSearch.ts(119,59): error TS4078: Parameter 'rawMatches' of exported function has or is using private name 'RegExpExecArray'.
src/vs/editor/common/model/textModelSearch.ts(507,29): error TS4055: Return type of public method from exported class has or is using private name 'RegExpExecArray'.
src/vs/editor/contrib/format/format.ts(20,38): error TS4020: 'extends' clause of exported class 'NoProviderError' has or is using private name 'Error'.
src/vs/platform/extensionManagement/node/extensionManagementService.ts(54,47): error TS4020: 'extends' clause of exported class 'ExtensionManagementError' has or is using private name 'Error'.
src/vs/platform/files/common/files.ts(574,41): error TS4020: 'extends' clause of exported class 'FileOperationError' has or is using private name 'Error'.
src/vs/platform/update/common/update.ts(15,9): error TS4033: Property 'date' of exported interface has or is using private name 'Date'.
src/vs/workbench/services/configuration/common/jsonEditing.ts(25,39): error TS4020: 'extends' clause of exported class 'JSONEditingError' has or is using private name 'Error'.
src/vs/workbench/services/configuration/node/configurationEditingService.ts(78,48): error TS4020: 'extends' clause of exported class 'ConfigurationEditingError' has or is using private name 'Error'.
src/vs/workbench/services/issue/common/issue.ts(16,31): error TS4075: Parameter 'dataOverrides' of method from exported interface has or is using private name 'Partial'.
src/vs/workbench/services/issue/electron-browser/workbenchIssueService.ts(29,30): error TS4073: Parameter 'dataOverrides' of public method from exported class has or is using private name 'Partial'.

[Ron Buckton (rbuckton)]

I'll have to follow up tomorrow on the TPromise issue, but the reason you are seeing other build errors with the custom 'lib' section is that none of those specific libraries entries references the 'es5' lib. If you add 'es5' to that list then the compile errors should go away.

[Johannes Rieken (jrieken)]

Ron Buckton (@rbuckton) when adding es5 to the lib section the promise errors appear again...

[Ron Buckton (rbuckton)]

Johannes Rieken (@jrieken) the "promise errors" are about using TPromise rather than Promise?

Regarding the other issue of using TPromise in ES6 async functions: We only allowed the use of other Promise-like definitions in ES5 since it did not have a native Promise implementation. For ES6 and onwards we only allow the use of the native Promise class for consistency with native support for async functions. We very likely should have only allowed a global Promise implementation in ES5 and require a shim to define the global implementation.

Is the issue that you are attempting to compile some parts of the project to both ES5 and ES6? If that is the case, your only recourse might be to define a global Promise shim for ES5 hosts that wraps WinJS.Promise in a fashion compatible with the ES6 native Promise definition.

[Johannes Rieken (jrieken)]

Is the issue that you are attempting to compile some parts of the project to both ES5 and ES6? If that is the case, your only recourse might be to define a global Promise shim for ES5 hosts that wraps WinJS.Promise in a fashion compatible with the ES6 native Promise definition.

Yeah, this is the problem: The monaco-editor and VS Code are in the same codebase/project and because the editor still supports IE11 we cannot leak 'modern' globals, like Symbol, Iterator etc, into that part of the project nor can we compile it down to es6. We have tackled this problem with to files: tsconfig.json and tsconfig.monaco.json. Now, for VS Code we would like to use ES6, more specifically we would like to emit modern JavaScript (using let, const, varargs, better async/await) and (optionally) use modern globals. I believe in TypeScript these two things are coupled, so either ES6 and modern globals or none of them. Ron Buckton (@rbuckton) correct me if I am wrong.

In fact, we have a shim for the native promise but the migration is quite expensive; there are subtle semantic differences which means care must be taken and then such changes are quite viral because WinJS.Promise and (native) promise are not assignable. Then, our native-promise-shim won't ever be compatible with the native promise+well-known-symbols because of the lacks of such well-known-symbols. Not sure what to do about that...

[Ron Buckton (rbuckton)]

We chose to require native Promise in ES6 and higher for async functions since native async functions in later versions do not give you the option to use a different Promise type.

However, it is possible to use a different Promise implementation at runtime (though not design time) by implementing a custom __awaiter helper. Here is an example:

• https://github.com/bterlson/ecmarkup/blob/master/src/Spec.ts#L34
• https://github.com/bterlson/ecmarkup/blob/master/src/awaiter.ts

[Johannes Rieken (jrieken)]

at runtime (though not design time) by implementing

That means you get compile errors, right?

[Johannes Rieken (jrieken)]

João Moreno (@joaomoreno) let's make a push next debt-week to reduce the amount of async-TPromise-usages?

[João Moreno (joaomoreno)]

Sure, I can lend a day or two. 👍

[Johannes Rieken (jrieken)]

Ok - tho great care must be taken... I have seen very unhappy thing happen when merging the worlds of TPromise and the native promise, esp. Promise.resolve(TPromise) will cause trouble...

[João Moreno (joaomoreno)]

Maybe we can come up with the general guidelines w/ pitfalls?

[Christof Marti (chrmarti)]

Johannes Rieken (@jrieken) new Promise(complete => complete(tPromise)) has the same issue as Promise.resolve(tPromise). If going from TPromise to the ES6 Promise cannot be avoided, wrapping the TPromise with a simple Thenable seems to work around the issue:

const es6Promise = Promise.resolve({
  then: function (fulfilled, failed) {
    return wp.then(fulfilled, failed);
  }
});

It would be best if we could come up with a solution that does not require tweaking each case. Especially when we gradually move from TPromises to ES6 Promises, we might often not be aware where our changes create new TPromise to ES6 Promise boundaries.

[Christof Marti (chrmarti)]

A JS Bin I have used to look into the promise incompatibility: http://jsbin.com/cavequh/edit?js,output

[Johannes Rieken (jrieken)]

We could try to add another overload to Promise.resolve which returns never when called with a winjs.promise.

[Ron Buckton (rbuckton)]

That means you get compile errors, right?

Not necessarily, it means that whatever Promise implementation you use has to at least align to the ES2015 Promise API.

Ok - tho great care must be taken... I have seen very unhappy thing happen when merging the worlds of TPromise and the native promise, esp. Promise.resolve(TPromise) will cause trouble...

I'm curious what issues you've seen. My prior experience with WinJS promises were that they violate guidelines for asynchronous APIs, often resulting in unpredictable timing of operations. However, it sounds like there's some other incompatibility when a native Promise adapts a WinJS Promise.

Also, another way to adapt a WinJS Promise to a native Promise might be the following:

new Promise((resolve, reject) => tPromise.then(resolve, reject));

[Ron Buckton (rbuckton)]

I ran the code sample from the JSBin in NodeJS and I think I understand why Promise.resolve(tPromise) doesn't work. WinJS replaces the value of tPromise.then on success, but Promise.resolve holds on to the then method of the promise it receives. This seems like a bug/issue in the WinJS Promise implementation as it probably shouldn't be replacing the values of public methods on an instance. The problems stem from the enter methods of state_success_notify, state_success, state_error_notify, and state_error where they replace the values of promise.done and promise.then.

[Christof Marti (chrmarti)]

If this starts occurring more often we might want to look into a permanent work around. I'm testing one here: #49034

[Johannes Rieken (jrieken)]

Next challenge... We have now a happy compile but we hit a road block when running es6-compiled-code because of how 3rd parties depend on our JavaScript code.
The problem is extending classes - which are now actually classes and not functions anymore. This a sample and typical for extensions.

var TelemetryReporter = /** @class */ (function (_super) {
    __extends(TelemetryReporter, _super);
    function TelemetryReporter(extensionId, extensionVersion, key) {
        var _this = _super.call(this, function () { return _this.toDispose.forEach(function (d) { return d && d.dispose(); }); }) || this;
        //...more...
        return _this;
    }
    //...more...
    return TelemetryReporter;
}(vscode.Disposable));

So, _super is this case an instance of vscode.Disposable which is declared and compiled as class, which means using call isn't allowed.

This puts us in a bad spot because we cannot ask extensions to change/recompile their code. Maybe Ron Buckton (@rbuckton) has some advise. Is there a way, maybe via emit-helpers, to change how super is being called? I think it would need some switch depending on the type of _super - (we made a similar change 835abba to our create-type util).

[Johannes Rieken (jrieken)]

Ron Buckton (@rbuckton) Could this be a case transformers? Like, we wanna use the normal es6-emitting but for classes we wanna use the es5-emitting. Does that sound reasonable?

[João Moreno (joaomoreno)]

Closing as won't happen any time soon.