search

medikoo / deferred

Modular and fast Promises implementation for JavaScript
ISC License
364 stars 20 forks source link

.then(success) : success called immediately on initially resolved promise #6

Closed timoch closed 12 years ago

timoch commented 12 years ago

Consider the following code : 

var sideEffect = false;
deferred(true).then(function(val) { 
    sideEffect = val; 
    console.log(val);  
}).end();
console.log(sideEffect);

This will output true true

I expect false true

Am I correct ? I have the feeling the callbacks for initially resolved promises should be scheduled on next tick. In case of a function returning a promise that sometime really needs to do async work but sometimes not, you can get different behavior.

I am not so experienced with Node but I assumed async callback-based functions are expected to wait until the next tick before calling their callback.

medikoo commented 12 years ago

@timoch this is the thing that was widely discussed recently and current status is:

  1. then just by itself is not an asynchronous function (is not like async function in Node.js which initiate some async action)
  2. Promise library like Deferred, should help with asynchronicity, but should not introduce any asynchronicity on its own
  3. It's the user that should decide whether he wants the callback to be run in nextTick and library shouldn't force it.
  4. Forcing callbacks to be called always in nextTick, affects performance (In fact old versions of Deferred did that, but later it was changed due to performance reasons).

So callback passed to then will be called when value is available, if it's available at the calling of then then callback will be called immediately.

Rule you should definitely follow is: Asynchronous functions that return promise MUST return unresolved promise

I've already configured many complex flows working with this behavior and it wasn't an issue. Usually we know whether we deal with resolved or unresolved promise. You're asking about that because it's practical problem for you or just theoretical concern?

timoch commented 12 years ago

@medikoo no practical issue just now but it's good to have in mind. The behavior is known and not subject to change. It would be a plus to add an extension that schedules resolution on next tick. It's trivial but it also gives an opportunity to specify the behavior of then() in a more explicit manner. Thanks for the quick answer

medikoo commented 12 years ago

@timoch Yes, current state is that without any valid reasoning backed by practical usage I'm definitely not going to change that behavior. Although I know that theoretically it may feel controversial.

I think it's important to actually try and practice it to find whether it's indeed an issue. I was also anxious (theoretical concerns) when first experimentally I've changed implementation, but then I found that's actually what I expect, and now I find a lot of logical reasoning (pointed above) that backs that model.

According to nextTick option, I think it's not needed. Still if someone really wants it, he may write: setImmediate.bind(null, callback), but again it's hard for me to come up with the case, when it would be useful

mwilliamson commented 11 years ago

Just as a note: this means that Deferred doesn't conform to the Promises/A+ spec (http://promises-aplus.github.io/promises-spec/). Specifically, 3.2.4 states:

then must return before onFulfilled or onRejected is called

I don't know whether that matters to you or not, but I thought I'd mention it on the off-chance you weren't aware.

medikoo commented 11 years ago

@mwilliamson yes, I'm totally aware. I actually don't agree with this part of spec. I've also discussed it once with Promise/A+ maintainers.

From my experience forcing that causes more trouble, than acknowledging that then may invoke received callback immediately. e.g. see issues in other implementations that force that:

https://github.com/cujojs/when/issues/135 https://github.com/kriskowal/q/pull/259 https://github.com/tildeio/rsvp.js/issues/66

.. and I haven't yet had issue with Deferred since I removed that rule (it was changed with v0.3.0 for performance reasons). The only case where it can be troublesome is when you as a programmer are not aware that then callback in Deferred may be invoked immediately.

mwilliamson commented 11 years ago

Fair enough, is it worth explicitly documenting the behaviour, calling out that it doesn't conform to Promises/A+?

medikoo commented 11 years ago

@mwilliamson I wouldn't put it that strong, core resolution logic adheres to Promise/A+ principles totally, and I'm strong supporter of those. With v0.7 I plan to add add section to a README explaining all the differences with clear reasoning for them