Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Promise 'then' incorrect? #457

Closed
Xotic750 opened this issue Aug 2, 2019 · 10 comments
Closed

Promise 'then' incorrect? #457

Xotic750 opened this issue Aug 2, 2019 · 10 comments

Comments

@Xotic750
Copy link
Contributor

Xotic750 commented Aug 2, 2019

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
Copy link
Collaborator

cscott commented Aug 2, 2019

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
Copy link
Contributor Author

Xotic750 commented Aug 3, 2019

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
Copy link
Collaborator

ljharb commented Aug 3, 2019

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
Copy link
Collaborator

ljharb commented Aug 3, 2019

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

@Xotic750
Copy link
Contributor Author

Xotic750 commented Aug 3, 2019

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
Copy link
Contributor Author

Xotic750 commented Aug 3, 2019

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

@ljharb
Copy link
Collaborator

ljharb commented Aug 3, 2019

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

@Xotic750
Copy link
Contributor Author

Xotic750 commented Aug 3, 2019

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
Copy link
Collaborator

ljharb commented Aug 3, 2019

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
Copy link
Contributor Author

Xotic750 commented Aug 3, 2019

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.

@ljharb ljharb closed this as completed Aug 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants