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

Issue #264 - Don't override QUnit.done #265

Closed
wants to merge 1 commit into from

Conversation

dcherman
Copy link
Contributor

Rather than overriding QUnit.done, register a callback with it so that
it continues to function normally. Overriding it was causing the
behavior of QUnit.done to differ from what was specified in the API docs
which caused some weird behavior with other plugins like QUnit.Composite.

Fixes #264.

Rather than overriding QUnit.done, register a callback with it so that
it continues to function normally.  Overriding it was causing the
behavior of QUnit.done to differ from what was specified in the API docs
which caused some weird behavior with other plugins like QUnit.Composite
@dcherman
Copy link
Contributor Author

There's no test suite to run and I'm not sure why QUnit.done was being overriden rather than simply used, so please take this PR with a grain of salt.

@Krinkle
Copy link
Member

Krinkle commented Mar 21, 2013

@dcherman It was bring overwritten because before 2012, that was the way to register the callbacks in QUnit. See also v1.0.0 history and qunitjs/qunit@1039a2c.

Assigning to the property actually still works fine (QUnit checks at run-time whether the property is a method from the prototype or an own property and if so, treat the property as a callback instead of looping through the internal list.

Though this works, it only works if either everything uses the new way or everything the old way. If something is using the old way (TestSwarm) and something else (your code) the new way, things go bad.

I'll land this though, it's about time!

@Krinkle Krinkle closed this Mar 21, 2013
@Krinkle
Copy link
Member

Krinkle commented Mar 21, 2013

Landed in fed8b59.

@Krinkle
Copy link
Member

Krinkle commented Mar 21, 2013

I'm checking for jQuery CLA signing. I should have asked for that before merging, could you sign jQuery's CLA now?

Please be sure to use the same name and email address as for committing with Git: http://contribute.jquery.org/CLA/

@dcherman
Copy link
Contributor Author

I signed it a while ago for some minor contributions to jQuery Core so I should be on there - happy to do so again if you need me to though

@Krinkle
Copy link
Member

Krinkle commented Mar 21, 2013

@dcherman Indeed you are (I see you now), my mistake. I must've missed it earlier (I think I was searching for "dchermen" instead of D C Herman). Thanks again!

@JamesMGreene
Copy link
Member

👍

@dcherman dcherman deleted the gh-264 branch April 13, 2013 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

TestSwarm doesn't play nicely with QUnit.Composite
3 participants