Skip to content
This repository has been archived by the owner on Dec 31, 2022. It is now read-only.

resolve(..) should call a thenable's then(..) async, not sync. #45

Closed
getify opened this issue Jun 9, 2015 · 5 comments
Closed

resolve(..) should call a thenable's then(..) async, not sync. #45

getify opened this issue Jun 9, 2015 · 5 comments

Comments

@getify
Copy link
Owner

getify commented Jun 9, 2015

This isn't a A+ requirement, it's an ES6 requirement.

The problem is, when I "fix" this, it causes lots of other test failures. And also the A+ test suite failures are intermittently different on each run, and sometimes the runner itself crashes. So, basically, this is impossible to track down.

This may in fact be a bug in not adhering to the ES6 spec, but I don't expect to be able to fix it, at least not any time soon. Spent a whole day on it so far, and failed. That's more time than I have to devote to such things.

More info: zloirock/core-js#78

For posterity, these are the offending lines, I believe:

_then.call(msg,
function $resolve$(){ resolve.apply(def_wrapper,arguments); },
function $reject$(){ reject.apply(def_wrapper,arguments); }
);

I believe that call is supposed to be deferred (with schedule(..)), but that breaks everything.

@Octane
Copy link

Octane commented Jun 11, 2015

I don't understand what the problem to call a thenable's then asynchronously.
In my implementation a thenable's then always will be called asynchronously, and it passing the Promise/A+ test suite.

Also with thenables there is another interesting issue.

@zloirock
Copy link

@Octane your issue looks similar mentioned, see where V8 get .then zloirock/core-js#78 (comment)

@getify
Copy link
Owner Author

getify commented Jun 11, 2015

I don't understand what the problem to call a thenable's then asynchronously.

The issue here is that NPO currently passes Promises/A+ when calling then(..) synchronously, but when I call it asynchronously, NPO has like 70+ test failures. I don't know why. I spent a day trying to figure it out, and failed.

@zloirock
Copy link

@getify see zloirock/core-js@17300b0 , NPO with same fix passes Promises/A+ test case. I can add PR.

@getify
Copy link
Owner Author

getify commented Jun 11, 2015

<3 <3 Huge thanks @zloirock!

I've packaged and released NPO 0.8.0-a with this fix.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants