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:

• replace usages of WinJS.Promise#cancel with the CancellationToken
• replace usages of the WinJS.Promise#then(_, _, onProgress) with a different progress-story
• replace usages of the WinJS.Promise#ctor
• replace usages of the WinJS.Promise#as|wrap|wrapError|join Replace WinJS.Promise.as with Promise.resolve #64597, Replace WinJS.Promise.join with Promise.all #64598, Replace WinJS.Promise.wrap with Promise.resolve #64596, Instantiate native promise instead of WinJS.Promise #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: #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! 🎆

[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️⃣

[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.