paulmillr / es6-shim

ECMAScript 6 compatibility shims for legacy JS engines
http://paulmillr.com
MIT License
3.11k stars 388 forks source link

Promise 'then' incorrect? #457

Closed Xotic750 closed 5 years ago

Xotic750 commented 5 years ago

I believe that

https://github.com/paulmillr/es6-shim/blob/master/es6-shim.js#L2569

and

https://github.com/paulmillr/es6-shim/blob/master/es6-shim.js#L2572

should be this.constructor

If you implement the shim and run the tests for subclass they will fail as this.mine = 'yeah' is not defined on the returned subclassed promise.

https://github.com/paulmillr/es6-shim/blob/master/test/promise/subclass.js#L27

cscott commented 5 years ago

Those lines match the spec, eg: https://www.ecma-international.org/ecma-262/6.0/#sec-promise.prototype.then

I believe the real issue is that the ES.SpeciesConstructor method is broken -- and I bet it has something to do with your runtime environment partially implementing Species support and thus fooling the shim. What platform are you running the tests on?

Xotic750 commented 5 years ago

I'm on Node 8.11.4, but I tried on 10 and 12 too. What I did was at

https://github.com/paulmillr/es6-shim/blob/master/es6-shim.js#L2624

I added globals.PromiseShim = PromiseShim;

Screenshot 2019-08-03 at 02 05 35

This gives me the implementation on the global.

then in subclass.js

I replaced Promise with PromiseShim to use the implementation.

Screenshot 2019-08-03 at 02 02 37

So, I kind of expected the implementation to pass all tests, perhaps there is is a good reason why it does not. But making those changes in my original post this.constructor, then the tests pass.

Why would I do this you ask, well I was interested about the inner workings of Promise, and I'm a tinkerer. :)

ljharb commented 5 years ago

The shims are not designed to be manually installed; if you just replace the global Promise, many things won't work - since that full replacement is only meant to be used on nodes that never had it to begin with.

ljharb commented 5 years ago

In other words, the es6-shim must be used in its entirety, or not at all.

Xotic750 commented 5 years ago

I appreciate that the library is to be used in its entirety. This is something that I came across while experimenting. Still, I find it strange that this should fail, and I thought it was worth mentioning.

Xotic750 commented 5 years ago

Ok, so PromiseShim does not define Symbol.species. That seems to be the answer?

ljharb commented 5 years ago

Sure - I assume because that symbol doesn't exist where Promise needs to be wholesale replaced.

Xotic750 commented 5 years ago

Yep, I added to PromiseShim

    if (supportsDescriptors) {
      Object.defineProperty(Promise, symbolSpecies, {
        configurable: false,
        enumerable: false,
        get: function() {
          return this;
        }
      });
    } else {
      object[symbolSpecies] = function() {
        return this;
      };
    }

https://www.ecma-international.org/ecma-262/6.0/#sec-get-promise-@@species and now all tests pass.

So my question is answered. I'm not saying anything is broken in the shim, as it works, but worth a note.

ljharb commented 5 years ago

If you’re suggesting there’s an engine which, after including the entire es6-shim, has Symbol.species but doesn’t have it on Promise, then i would consider that a bug.

Xotic750 commented 5 years ago

No, I'm not. I was just running the raw implementation (which as you said, is not designed for this purpose) for interest sake, and came across this and was curious as to why it was failing. Now I understand why, and learnt some stuff in the process. As you rightly point out, this case should never happen within the design of es6-shim.