@alexandrudima

AFAICT, the following two snippets produce the same result

Well, in the one case a native promise is created and in the other not, so even though both might produce the same result, your solution still mixes native promises and WinJS promises.

I can see if TPromise.wrap(<native promise>) causes the same issue with this._state.then is not a function to verify that assumption.

@alexandrudima with TPromise.wrap(import('xyz')) I am still hitting an error this._state.then is not a function. Steps:

• uncomment the 2 console.log statements in bulkEditService (here)
• change doMoveItemToTrash to the implementation leveraging TPromise.wrap(import('electron')) (see below)
• run scripts/test-integration.sh
• see output appearing at the end ("TypeError: this._state.then is not a function", source: file:///Users/bpasero/Development/Microsoft/monaco/out/vs/workbench/services/bulkEdit/electron-browser/bulkEditService.js (458))

private doMoveItemToTrash(resource: uri): TPromise<void> {
	const absolutePath = resource.fsPath;

	return TPromise.wrap(import('electron')).then(electron => {
		const result = electron.shell.moveItemToTrash(absolutePath);
		if (!result) {
			return TPromise.wrapError(new Error(isWindows ? nls.localize('binFailed', "Failed to move '{0}' to the recycle bin", paths.basename(absolutePath)) : nls.localize('trashFailed', "Failed to move '{0}' to the trash", paths.basename(absolutePath))));
		}

		this._onAfterOperation.fire(new FileOperationEvent(resource, FileOperation.DELETE));

		return void 0;
	});
}

This does not happen with my proposed solution that avoids mixing native promises with WinJS promises where this can happen if the native promise resolves directly (the electron import is already resolved). I think this issue could also reproduce with other node.js imports that are loaded multiple times in different locations where the second load finds the module already in the cache.

I can reproduce and here is a reduction:

Refinement 1

	let p1 = TPromise.wrap<number>(new Promise<number>(function(c, e) { c(1); }));
	Promise.all([p1]);
	// ==> Uncaught TypeError: this._state.then is not a function

Refinement 2: drop Promise.all

	let p1 = TPromise.wrap<number>(new Promise<number>(function(c, e) { c(1); }));
	let thenFunc = p1.then.bind(p1);
	setTimeout(() => {
		thenFunc(() => {
			console.log(`finished`);
		});
	}, 0);
	// ==> Uncaught TypeError: this._state.then is not a function

Refinement 3: do not use native Promise at all!

	let c;
	let p1 = new TPromise(function(_c, e) { c = _c; });
	let thenFunc = p1.then.bind(p1);
	setTimeout(() => {
		c(1);
		thenFunc(() => {
			console.log(`finished`);
		});
	}, 0);
	// ==> Uncaught TypeError: this._state.then is not a function

IMHO dropping native promises in imports will only cover a subset of refinement 3. This exception will occur anytime someone captures the then method before the winjs promise is resolved...

The PR #49034 from Christof actually fixes this and IMHO we should take that, as winjs is broken unrelated to native promises --- se refinement 3.

The root cause is that winjs overrides a promise's .then method with a different .then method at the time the promise is resolved. So, if anyone has captured the .then method they will have a stale version.

@chrmarti any reason you did not push your solution forward?

@bpasero I briefly discussed with @chrmarti on Friday, he was not sure if this was the right solution. But now we have more proof (beyond native promises) that the root cause is the then function being overwritten from the state.

I suggest we go forward and merge @chrmarti's PR in debt week.