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

$.when, $.progress and already resolved deferred #1894

Closed
nicolashenry opened this Issue Dec 2, 2014 · 7 comments

Comments

Projects
None yet
4 participants
@nicolashenry
Contributor

nicolashenry commented Dec 2, 2014

This code :

$.when($.Deferred().notify(1).resolve(), 
$.Deferred().notify(2).resolve(), 
$.Deferred().notify(3).resolve())
.progress(function() {
    console.log(arguments);
});

displays "[1,2,undefined]".
I would expect to have "[1,2,3]" as a result because this behavior with the last notify does not appear to be described in the documentation. If any of the deferred objects is not already resolved, the display is "[1,2,3]".

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Dec 2, 2014

Member

No matter now many (N>1) are combined, the last one never gets a progress notification. http://jsbin.com/muhuxireko/1/edit?js,console

For reference, the docs say:

When the Deferred is resolved or rejected, progress callbacks will no longer be called, with the exception that any progressCallbacks added after the Deferred enters the resolved or rejected state are executed immediately when they are added, using the arguments that were passed to the .notify() or notifyWith() call. http://api.jquery.com/deferred.progress/

When deferred.notify is called, any progressCallbacks added by deferred.then or deferred.progress are called. Callbacks are executed in the order they were added. Each callback is passed the args from the .notify(). Any calls to .notify() after a Deferred is resolved or rejected (or any progressCallbacks added after that) are ignored. http://api.jquery.com/deferred.notify/

Frankly I wouldn't have even expected the combined Deferred to report progress from the individual ones. These kind of situations are probably some of many reasons why standard Promise punted on progress. Others, thoughts?

Member

dmethvin commented Dec 2, 2014

No matter now many (N>1) are combined, the last one never gets a progress notification. http://jsbin.com/muhuxireko/1/edit?js,console

For reference, the docs say:

When the Deferred is resolved or rejected, progress callbacks will no longer be called, with the exception that any progressCallbacks added after the Deferred enters the resolved or rejected state are executed immediately when they are added, using the arguments that were passed to the .notify() or notifyWith() call. http://api.jquery.com/deferred.progress/

When deferred.notify is called, any progressCallbacks added by deferred.then or deferred.progress are called. Callbacks are executed in the order they were added. Each callback is passed the args from the .notify(). Any calls to .notify() after a Deferred is resolved or rejected (or any progressCallbacks added after that) are ignored. http://api.jquery.com/deferred.notify/

Frankly I wouldn't have even expected the combined Deferred to report progress from the individual ones. These kind of situations are probably some of many reasons why standard Promise punted on progress. Others, thoughts?

@dmethvin dmethvin added the Deferred label Dec 2, 2014

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Dec 3, 2014

Member

@jaubourg any input?

Member

dmethvin commented Dec 3, 2014

@jaubourg any input?

@jaubourg

This comment has been minimized.

Show comment
Hide comment
@jaubourg

jaubourg Dec 4, 2014

Member

I cannot properly investigate since I'm travelling until monday. I'll look into it then.

Member

jaubourg commented Dec 4, 2014

I cannot properly investigate since I'm travelling until monday. I'll look into it then.

@nicolashenry

This comment has been minimized.

Show comment
Hide comment
@nicolashenry

nicolashenry Dec 9, 2014

Contributor

I think the problem comes from here (deferred.js#L134) :

resolveValues[ i ].promise()
    .done( updateFunc( i, resolveContexts, resolveValues ) )
    .fail( deferred.reject )
    .progress( updateFunc( i, progressContexts, progressValues ) );

The progress update should perhaps be done first :

resolveValues[ i ].promise()
    .progress( updateFunc( i, progressContexts, progressValues ) )
    .done( updateFunc( i, resolveContexts, resolveValues ) )
    .fail( deferred.reject );
Contributor

nicolashenry commented Dec 9, 2014

I think the problem comes from here (deferred.js#L134) :

resolveValues[ i ].promise()
    .done( updateFunc( i, resolveContexts, resolveValues ) )
    .fail( deferred.reject )
    .progress( updateFunc( i, progressContexts, progressValues ) );

The progress update should perhaps be done first :

resolveValues[ i ].promise()
    .progress( updateFunc( i, progressContexts, progressValues ) )
    .done( updateFunc( i, resolveContexts, resolveValues ) )
    .fail( deferred.reject );
@jaubourg

This comment has been minimized.

Show comment
Hide comment
@jaubourg

jaubourg Dec 9, 2014

Member

I think that's the problem too. Wanna make a PR for this with a nice unit test to boot @nicolashenry? :)

Member

jaubourg commented Dec 9, 2014

I think that's the problem too. Wanna make a PR for this with a nice unit test to boot @nicolashenry? :)

@nicolashenry

This comment has been minimized.

Show comment
Hide comment
@nicolashenry

nicolashenry Dec 9, 2014

Contributor

Is it okay that way?

Contributor

nicolashenry commented Dec 9, 2014

Is it okay that way?

@dmethvin dmethvin removed the Needs review label Dec 10, 2014

@dmethvin dmethvin added this to the 3.0.0 milestone Dec 10, 2014

@dmethvin dmethvin self-assigned this Dec 10, 2014

@markelog markelog closed this in ab20d9d Dec 25, 2014

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Dec 25, 2014

Member

Merged! Thank you

Member

markelog commented Dec 25, 2014

Merged! Thank you

@dmethvin dmethvin modified the milestones: 1.12/2.2, 3.0.0 Jan 7, 2016

@cssmagic cssmagic referenced this issue May 18, 2016

Open

jQuery #5

@lock lock bot locked as resolved and limited conversation to collaborators Jun 19, 2018

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