async/sync invocation of domready handlers is inconsistent

[jasonkarns]

Assuming the standard scenario where an event handler is registered prior the the document being ready, the handlers are (as intended) fired asynchronously.

However, when a domready handler is registered after the document is already ready, it is executed synchronously. This makes usage of this library unpredictable and inconsistent. It's generally recommended (I daresay best practice; but what a loaded phrase) that callbacks that may be invoked asynchronously should always be executed asynchronously.

I propose any handlers registered after domready should be invoked via process.nextTick in node or setTimeout(fn, 0).

I'm more than happy to submit a PR if the change will be accepted.

[juliangruber]

+1

[ded]

i'm sorry that i don't follow this correctly. all callbacks are async. if the DOM has loaded already, it fires back, if it hasn't, it waits. no matter what, the API is as such, which by nature, is always async

domready(function () {
  // do stuff...
})

[jasonkarns]

Callbacks can be synchronous or asynchronous. They are not automatically async by virtue of being a callback. Example:

function callbackTaker (cb){
  cb();
}

callbackTaker(function(){
  // I am executed synchronously by `callbackTaker`
  // ie, callbackTaker is still in the callstack at this point
});

Another example showing order of execution under the two cases of dom being loaded or not at time of callback registration:

// if DOM not loaded
var after = false;
domready(() => {
  assert(after); // SUCCEEDS
});
after = true;

// if DOM already loaded
var after = false;
domready(() => {
  assert(after); // FAILS because this callback is executed synchronously
});
after = true;

A reference by which I stand that APIs should either invoke callbacks synchronously or asynchronously, but never both: http://blog.izs.me/post/59142742143/designing-apis-for-asynchrony (lots of other linked sources in that post as well)

[jasonkarns]

For clarification, the offending line is https://github.com/ded/domready/blob/1855db0747a25a25dcbe7e782919696c44f82379/src/ready.js#L27

If loaded, fn is invoked directly. To make it async, it would need to be invoked via setTimeout(fn, 0)

[ded]

i gotcha. we can just wrap it in setTimeout and that will do the trick