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

Deferred callback parameters fix #1922

Merged
merged 3 commits into from
Apr 3, 2014
Merged

Deferred callback parameters fix #1922

merged 3 commits into from
Apr 3, 2014

Conversation

MaceWindu
Copy link

Deferred.always,Deferred.done,Deferred.progress,Deferred.fail should take callbacks as parameters.

@apare
Copy link
Contributor

apare commented Mar 28, 2014

👍

@@ -288,6 +288,13 @@ interface JQueryGenericPromise<T> {
}

/**
* Interface for the JQuery promise/deffered callbacks
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this read "JQuery promise / deferred callbacks"?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, English is not my strong side

@johnnyreilly
Copy link
Member

Hi @MaceWindu,

Thanks for contributing!

If I read this correctly this PR makes sure that only "then-able" style functions can be passed to the various deferred / promise methods.

On the face of it seems entirely valid. And the existing tests are passing (with a minor amend to one). That said, I'm not too close to the jQuery Promise / Deferred implementation. Does someone else on the DT team want to cast their eyes over this and merge if they're happy?

Cheers.

@MaceWindu
Copy link
Author

Problem with previous implementation was that type parameter was declared as callback and it resulted in resolve/reject/progress/notify methods parameter to have a callback type too. This is wrong.

Now situation is better, but still not perfect. E.g. if you pass parameter of type1 to resolve(), parameter of type2 to reject(), and parameter of type3 to progress() - you will need Deferred definition with 3 type parameters (if we type only first parameter for each function, because TS doesn't support variable number of type parameters unfortunately) and you cannot have typed definition for always() - it will be type1 or type2.

@johnnyreilly
Copy link
Member

Thanks for the clarification @MaceWindu

Would someone else on the DT team be happy to eyeball this? @vvakame @Diullei @basarat @Bartvds - any takers?

@vvakame
Copy link
Member

vvakame commented Apr 1, 2014

@johnnyreilly I do not say anything because i do not use... sorry.

suggestion: https://github.com/borisyankov/DefinitelyTyped/blame/d89a73caaf7aa99158996a0cb1fa42fee6e2cd8d/jquery/jquery.d.ts#L300
I think that we should listen to the opinion of @AndrewGaspar.

@ryanbnl
Copy link

ryanbnl commented Apr 1, 2014

+1, this would fix problems on my project :)

@AndrewGaspar
Copy link
Contributor

These changes seem fine to me. I'm kind of confused as to why we had the T and T[] arguments before... that seems completely wrong.

@apare
Copy link
Contributor

apare commented Apr 2, 2014

The only isue :
var dfd = $.Deferred();
dfd.done( [ fn1, fn2 ], fn3 );

Like the documentation said : doneCallbacks is a function, or array of functions, that are called when the Deferred is resolved. Done can take function and array of function... but I don't think anyone uses array as arguments.

@johnnyreilly
Copy link
Member

The consensus seems to be"yes" so let's merge!

johnnyreilly added a commit that referenced this pull request Apr 3, 2014
Deferred callback parameters fix
@johnnyreilly johnnyreilly merged commit a4de992 into DefinitelyTyped:master Apr 3, 2014
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

Successfully merging this pull request may close these issues.

None yet

6 participants