This polyfill does not play nice with other Promise polyfills

[l1bbcsg]

Namely, it does not respect any attempts to patch its functions when creating new Promises within itself. I tried to apply Promise.prototype.finally and unhandledrejection event atop this polifyll and it produced really weird behaviour.

Simplified, this polyfill works basically like this:

class Constructor {
	// implementation
}
window.Promise = Constructor;

The two polyfills I mentioned and likely most others work (again, simplified to pseudo-code) either like this:

const originalThen = window.Promise.prototype.then;
window.Promise.prototype.then = () => {
	// do something extra
	originalThen.call(this);
}

or like this:

class PatchedPromise extends Promise {}
window.Promise = PatchedPromise

They do their part respecting original Promise implementation regardless of what it was. And they works fine with instances created with new Promise (class provided by es6-promise). I.e. new Promise(foo).finally(bar) works as expected.

However, most functions within es6-promise when creating new Promise instances use new Constructor instead on new Promise and these objects obviously do not have their methods patched by other polyfills. Static Promise.resolve being the most obvious example: https://github.com/stefanpenner/es6-promise/blob/master/lib/es6-promise/promise/resolve.js#L45 so Promise.resolve().finally() throws since Constructor instance does not have finally property.

The cleanest solution is probably using new this.constructor instead of new Constructor

[stefanpenner]

However, most functions within es6-promise when creating new Promise instances use new Constructor instead on new Promise

This should be fine, and also required to make subclassing work. Without subclassing in all those cases Constructor should === Promise

[stefanpenner]

I tried to apply Promise.prototype.finally

Just tested this my self, and it seems to work fine.

const Promise = require('es6-promise').Promise

delete Promise.prototype.finally; // just in-case it exists (master has it, but we want the polyfill to work)

if (typeof Promise !== 'function') {
	throw new TypeError('A global Promise is required');
}

if (typeof Promise.prototype.finally !== 'function') {
	var speciesConstructor = function (O, defaultConstructor) {
		if (!O || (typeof O !== 'object' && typeof O !== 'function')) {
			throw new TypeError('Assertion failed: Type(O) is not Object');
		}
		var C = O.constructor;
		if (typeof C === 'undefined') {
			return defaultConstructor;
		}
		if (!C || (typeof C !== 'object' && typeof C !== 'function')) {
			throw new TypeError('O.constructor is not an Object');
		}
		var S = typeof Symbol === 'function' && typeof Symbol.species === 'symbol' ? C[Symbol.species] : undefined;
		if (S == null) {
			return defaultConstructor;
		}
		if (typeof S === 'function' && S.prototype) {
			return S;
		}
		throw new TypeError('no constructor found');
	};

	var shim = {
		finally(onFinally) {
			var promise = this;
			if (typeof promise !== 'object' || promise === null) {
				throw new TypeError('"this" value is not an Object');
			}
			var C = speciesConstructor(promise, Promise); // throws if SpeciesConstructor throws
			var handler = typeof onFinally === 'function' ? onFinally : () => {};
			var newPromise = Promise.prototype.then.call(
				promise,
				x => new C(resolve => resolve(handler())).then(() => x),
				e => new C(resolve => resolve(handler())).then(() => { throw e; })
			);
			return newPromise;
		}
	};
	Object.defineProperty(Promise.prototype, 'finally', { configurable: true, writable: true, value: shim.finally });
}

Promise.resolve().finally().finally().finally()

[stefanpenner]

@l1bbcsg it would be helpful if an executing example of a problem could be provided, that way we can be sure to talk about the same thing.

[stefanpenner]

I do expect issues to arise with the unhandledRejection stuff, and am tracking some of that over here: #172 But an actual executing example of what you see would be handy.

[l1bbcsg]

It seems I jumped to conclusions regarding finally. It does seem to work as expected.
It only adds a property to Promise constructor which is the same as Constructor, so no reason for it not to work now that I think of it. Sorry for consusion. I guess my problem with it was a bit deeper: I used its transpiled version and the other polyfill might have intervened – will investigate, but it should be safe to dismiss finally report, especially considering finally just landed to es6-promise.

The original issue still stands though I believe since unhandledrejection does suffer from this problem and any other attempt to inherit and redefine Promise should as well:

delete global.Promise; 
global.Promise = require('es6-promise').Promise;

// The following is browser-unhandled-rejection/dist/bundle.es.js 
// with contents of dispatchUnhandledRejectionEvent replaced with console.log
// for the purposes of testing since it relies on browser environment and won't work in node

var OriginalPromise = Promise;

/**
 * ES5 subclassing is used per:
 * https://github.com/rtsao/browser-unhandled-rejection/issues/1
 * https://kangax.github.io/compat-table/es6/#test-Promise_is_subclassable
 *
 * Adapted from: https://gist.github.com/domenic/8ed6048b187ee8f2ec75
 */
var InstrumentedPromise$1 = function Promise(resolver) {
  if (!(this instanceof InstrumentedPromise$1)) {
    throw new TypeError('Cannot call a class as a function');
  }
  var promise = new OriginalPromise(function (resolve, reject) {
    return resolver(resolve, function (arg) {
      OriginalPromise.resolve().then(function () {
        if (promise._hasDownstreams === undefined) {
          dispatchUnhandledRejectionEvent(promise, arg);
        }
      });
      return reject(arg);
    });
  });
  promise.__proto__ = InstrumentedPromise$1.prototype;
  return promise;
};

InstrumentedPromise$1.__proto__ = OriginalPromise;
InstrumentedPromise$1.prototype.__proto__ = OriginalPromise.prototype;

InstrumentedPromise$1.prototype.then = function then(onFulfilled, onRejected) {
  var next = OriginalPromise.prototype.then.call(this, onFulfilled, onRejected);
  this._hasDownstreams = true;
  return next;
};

function dispatchUnhandledRejectionEvent(promise, reason) {
  console.log('dispatchUnhandledRejectionEvent', promise, reason)
}

function needsPolyfill() {
  return typeof PromiseRejectionEvent === 'undefined';
}

function polyfill() {
  Promise = InstrumentedPromise$1;
}

function auto() {
  if (needsPolyfill()) {
    polyfill();
  }
}


// Test
auto();

// this triggers unhandledrejection event
new Promise((resolve, reject) => {
	reject('simple reject');
});

// this does not
Promise.reject('static reject');

[stefanpenner]

Thanks. I agree, and your concerns are already tracked by:

• support real subclass + constructor tampering #172
• Possibly unhandled error should be thrown #70

Maybe this holiday season I'll have some cycles to improve these things.