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

Async consistency #1

Closed
cowsrule opened this issue Jan 16, 2017 · 6 comments
Closed

Async consistency #1

cowsrule opened this issue Jan 16, 2017 · 6 comments

Comments

@cowsrule
Copy link

Minor code-review point after seeing a link to you lib.

It's (almost) always a good idea to guarantee that your callback will be either consistently async or consistently sync in every case it is used. Not having this guarantee can cause hard to find and hard to reproduce bugs in your code.

In the case that the DOMContentLoaded event is missed, the callback will be sync. In the case that the event has not yet been fired, the callback will be async. Consider wrapping the explicit call to the callback in a setTimeout to guarantee that the invocation of the callback will always be async.

@lukechilds
Copy link
Owner

I was aware that's best practise, however I assumed that a Promise executer ran async so everything inside was also async. Turn out that's not correct.

Fixed: 1284c58

Thanks for the feedback 👍

@Yvem
Copy link

Yvem commented Jan 31, 2017

Disagree.

The callback (node style) should indeed be always async, but not the promise resolution, which is expected to be potentially synchronous by the promise spec, doesn't have too.

@lukechilds lukechilds mentioned this issue Feb 1, 2017
@lukechilds
Copy link
Owner

@Yvem Good point. Fixed in v1.2.5.

@Yvem
Copy link

Yvem commented Feb 1, 2017

When reading the code, it seems that the return value from the callback has an effect on the resolved value of the promise. Is it by design ?

@lukechilds
Copy link
Owner

lukechilds commented Feb 2, 2017

That's an undocumented side effect.

With the new change in v1.2.5 the timeoutID for the callback will be the resolved value of the promise. I don't really expect people to be mixing callbacks and promises though.

@lukechilds
Copy link
Owner

@Yvem This was annoying me, just fixed the weird resolve value behaviour: #11

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