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

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

Closed
timoch opened this issue Nov 6, 2012 · 7 comments
Closed
Assignees

Comments

@timoch
Copy link

timoch commented Nov 6, 2012

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.

@ghost ghost assigned medikoo Nov 8, 2012
@medikoo
Copy link
Owner

medikoo commented Nov 8, 2012

@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?

@medikoo medikoo closed this as completed Nov 8, 2012
@timoch
Copy link
Author

timoch commented Nov 9, 2012

@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
Copy link
Owner

medikoo commented Nov 9, 2012

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

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

medikoo commented Apr 15, 2013

@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:

cujojs/when#135
kriskowal/q#259
tildeio/rsvp.js#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
Copy link

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

@medikoo
Copy link
Owner

medikoo commented Apr 15, 2013

@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

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