stefanpenner / es6-promise

A polyfill for ES6-style Promises
MIT License
7.3k stars 593 forks source link

Intercepting `Promise.resolve` fails in `enumerator.js#L82` #335

Open codeworrior opened 6 years ago

codeworrior commented 6 years ago

In a testing framework that we use, Promise.resolve is intercepted (among others) to implement some bookkeeping for pending promises, timers, requests etc. The intercepting code looks something like this:

(function() {
  const originalResolve = Promise.resolve;  // *1*
  Promise.resolve = function(value) {

    // propagate the current call to the original function, 
    // using the same context and argument
    let result = originalResolve.call(this, value); // *2*

    // bookkeeping is implemented around the original call, doesn't matter here
    return result;   

  };
}();

Recently, we tried to upgrade es6-promise from 2.3.0 to 4.2.4 to benefit from Promise.prototype.finally. But then, the above intercepting code failed with a

TypeError: Constructor is not a constructor at resolve$1 (es6-promise.js:231:17)

The reason seems to be that es6-promise's current Promise.resolve implementation relies on this (to allow inheritance), but the Enumerator.prototype._eachEntry implementation ignores this fact here in combination with line 82.

To cite from #286 (IMO applies to resolve as well):

If you wish to destructure all, it can be, but must be first bound.

Well, I understand that this applies to our test framework (line *1) and binding originalResolve would fix the issue. But I think, the strategy of calling the originalResolve with the current this and argument (line *2) is also fine and a bit more universal. It even should work if someone would re-use the wrapped Promise.resolve in a subclass of Promise.

IMO it would be fair if Enumerator.prototype._eachEntry either would follow the "if you wish to destructure ... it first must be bound" rule itself or would call Constructor.resolve in line 82.

What do you think?