Vscode: WinJS.Promise removal plan

[jrieken]

We are heavy users of WinJS.Promise and that's not good. We started with the monaco-editor before native promises existed, so WinJS.Promises came in handy. However, times have changed, native promises are real and sticking with WinJS.Promises became a dead end. Time for change...

Migrating from WinJS.Promise to the native promises isn't a find/replace-job because of some incompatibilities, namely the runtime-behaviour and different APIs. In a first wave we want to focus on two elephants:

• [x] replace usages of WinJS.Promise#cancel with the CancellationToken
• [x] replace usages of the WinJS.Promise#then(_, _, onProgress) with a different progress-story
• [x] replace usages of the WinJS.Promise#ctor
• [x] replace usages of the WinJS.Promise#as|wrap|wrapError|join #64597, #64598, #64596, #62716

In more detail

WinJS.Promise#cancel

Calling winjsPromise.cancel() does two things: (1) it calls a cancel-handler that you provide when constructing a winjs-promise and (2) it puts the promise in the rejected-state with a cancelled-error.

The native promise doesn't support cancellation and it needs to be replaced it with our own solution. For extensions we already offer CancellationTokenSource and CancellationToken and we want to adopt it for "internal" use too. It's important to know that a cancellation token is just a hint and that a consumer might ignore it and proceed as before. In contrast to the winjs-promise-behaviour that means you must not end-up in the rejection-handler but you might end-up in the success-handler. That's fine as long as you can handle it.

Because all of our code is written (and tested) with the winjs-promise-behaviour in mind we have added a util that makes adoption easy: CancelablePromise and createCancelablePromise. Checkout the initial commit to understand how to use it: https://github.com//Microsoft/vscode/commit/727c6f3537d732269db30cbb9efd496073351586. The contract is that calling cancel puts the promise into the rejected-state using the well-known cancelled-error.

onProgress

The then-function of winjs-promises can accept three callbacks and the latter is used for progress events. Those can fire multiple times during promise execution. Again, nothing the native promise supports and something we need to replace with our own code. There is PR in which this effort is tracked: https://github.com/Microsoft/vscode/pull/53487

Differences in runtime behaviour

The winjs-promise shows different runtime behaviour then native promises and when switching to native promises you should be aware of that. In contract to native promises, a winjs-promise that is resolved in its executor-function calls its then-handlers immediately. Check with these tests that outline the differences: https://github.com/Microsoft/vscode/blob/11c5b9381b8d1db012055201d48cc4d37299afce/src/vs/base/test/common/winjs.polyfill.promise.test.ts#L36

[jrieken]

fyi @Microsoft/vscode we are down to 56 references to WinJS.Promise#cancel and the goal is 0 references. If you have spare cycles during the debt week and if you are a cancel-user then please migrate your code.

[joaomoreno]

Progress is now 100% removed in master! :fireworks:

[jrieken]

WinJS.Promise#cancel is now removed in master 🎆

[jrieken]

@Microsoft/vscode With the removal of progress and cancel we have made the WinJS.Promise compatible with native promises. A WinJS.Promise is now a super-type of native promises which means you can return or assign a native promise where a winjs-promise is expected. That should enable a smooth transition into the native promise world.

❗️ A word of caution: The timings of winjs-promises and native promises differ! A winjs-promise that is resolved synchronously will call its then-handler immediately (instead of waiting for the next tick). We have some unit tests that outline the differences. https://github.com/Microsoft/vscode/blob/3c2dbc17b84191950cd61b9c8d95776abdc672f6/src/vs/base/test/common/winjs.polyfill.promise.test.ts#L13-L39

Please keep this in mind when replacing winjs-promises with native promises. Also note that this is not only for constructor calls but also when calling resolve.

[tnclark8012]

The above behavior is an issue when using Monaco with Zone.js and Angular. ZoneAwarePromise's all function is as follows:

ZoneAwarePromise.all = function (values) {
            var resolve;
            var reject;
            var promise = new this(function (res, rej) {
                resolve = res;
                reject = rej;
            });
            var count = 0;
            var resolvedValues = [];
            for (var _i = 0, values_2 = values; _i < values_2.length; _i++) {
                var value = values_2[_i];
                if (!isThenable(value)) {
                    value = this.resolve(value);
                }
                value.then((function (index) { return function (value) {
                    resolvedValues[index] = value;
                    count--; // <-- because WinJS invokes this callback immediately, the next promise will have the same index value
                    if (!count) {
                        resolve(resolvedValues);
                    }
                }; })(count), reject);
                count++;
            }
            if (!count)
                resolve(resolvedValues);
            return promise;
        };

This implementation is relying on the microtask that's missing from the WinJS completed promise. As a result, calling ZoneAwarePromise.all([winJsPromise1, winJsPromise2]) will be resolved with the array [result2] instead of the expected [result1, result2]. Here's a Fiddle demonstrating the issue.

I noticed the issue when using Monaco + Angular and setting error markers on the editor. The hover text rendered 'undefined' instead of my intended message text because of this line in HtmlContentRenderer -- value is a WinJS promise and withInnerHTML is a ZoneAwarePromise.

@jrieken any suggestions for handling this? Is sync then-handler invocation a desired/necessary behavior? If so, I suppose we'll have to try and patch Zone.js

[jrieken]

@jrieken any suggestions for handling this? Is sync then-handler invocation a desired/necessary behavior? If so, I suppose we'll have to try and patch Zone.js

Unfortunately not. I'd patch Zone.js or use native Promise.all instead

[jrieken]

Only 6043 references to WinJS.Promise are left...

Good news is that after the removal of progress and cancel the migration is simple. Whereever a winjs-promise is being returned a native-promise can and should be returned. Please invest some time to migrate your code:

• new WinJS.Promise(callback) -> new Promise(callback)
• WinJS.Promise.as -> Promise.resolve
• WinJS.Promise.wrap -> Promise.resolve
• WinJS.Promise.wrapError -> Promise.reject
• WinJS.Promise.join -> Promise.all
• WinJS.Promise.any -> Promise.race

When migrating think of the differences in the runtime behaviour! This affects promises that are synchronously resolved (via the constructor callback or via above utils) and that have a then-handler (all details here)

// NATIVE promise behaviour
const promise = new Promise(resolve => {
    // 1️⃣
    resolve(null);
}).then(() => {
    // 3️⃣
});
// 2️⃣

// WINJS promise behaviour
const promise = new WinJSPromise(resolve => {
    // 1️⃣
    resolve(null);
}).then(() => {
    // 2️⃣
});
// 3️⃣

[jrieken]

@Microsoft/vscode GH can only assign 10 people to an issue but this is affects everyone

[isidorn]

@jrieken thanks for the ping, I just started on this. One question though: should we keep the method signitures to be TPromise, since if we also migrate those than this will spread virally.
Currently I can return a native promise when the signiture says TPromise, but not the other way around.
Edit: accidently closed, sorry

[jrieken]

Currently I can return a native promise when the signiture says TPromise, but not the other way around.

Correct - that means you can use native promise where TPromise is expected but not the other way around (which is what we actually want). I see the challenge with the viral spread when changing signature. Two options: 1 power through when feasible e.g. it is your code and/or you have sound understand of things, or 2 use Thenable as a stepping stone - it's compatible to both in both directions.

[isidorn]

@jrieken yeah I will go with Thenable for now, seems to work pretty nicely.

[isidorn]

Geeez did GitHub just change that I can no longer reference an issue in a commit without the commit closing the issue.
Before I could say

53526 wihtout closing the issue and

fixes #53526 to close it

Now it seems that GH closes the issue in both cases

[sandy081]

@isidorn Not for me.. I referred the issue in my commit and it is still open.

[sandy081]

One approach I am following while moving away from TPromise to Promise in my area:

• If in my code, there is a dependency to other services which return TPromise instead of wrapping them with Promise.resolve(TPromise) I am changing my method to async

https://github.com/Microsoft/vscode/blob/46c97fe4139685c3d37ddb5f14660d7f5bbc0244/src/vs/workbench/services/configuration/node/configurationEditingService.ts#L158

[jrieken]

@sandy081 be careful with that async. the state machine TS generates for it is known to be slow

[eamodio]

@jrieken Doesn't node 8x support native async/await?

[jrieken]

@eamodio yeah, currently we have to downlevel compile to es5, partly because of our mono-source approach and IE11 support for monaco but mostly because TypeScript only compiles to ES6 when native promises are used. So, once the promise migration happened we can tackle the async/await problem. That will be a big problem but that's another story... (more background https://github.com/Microsoft/vscode/pull/47144)

[eamodio]

Ah. Thanks for the info!

[nojvek]

Makes me so glad to see WinJs -> native promises.

I hope to see more async awaits in vscode in the future.

[joaomoreno]

An era comes to an end.