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

wrap jquery's deferred[ resolveWith | rejectWith | notifyWith ] #268

Merged
merged 2 commits into from
Jul 25, 2015

Conversation

matghaleb
Copy link

New implementation of previous PR #266

@@ -72,4 +72,19 @@ $.ajax = function ravenAjaxWrapper(url, options) {
}
};

var _oldDeferred = $.Deferred;
$.Deferred = function ravenDeferredWrapper(func) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't we want to check that $.Deferred is not undefined first? Or does it not matter?

Copy link
Contributor

Choose a reason for hiding this comment

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

Worst case is that it introduces (a broken) $.Deferred when it wasn't there before. But yeah, I'd probably leave it if it's not defined.

Aside, $.Deferred was introduced in jQuery 1.5 in 2011.

@mattrobenolt
Copy link
Contributor

@matghaleb Is it possible to supply a test case for this? Even if it's a jsfiddle or something functional.

This is a very highly used plugin and I'd be afraid of breaking it without actually testing it.

@matghaleb
Copy link
Author

@mattrobenolt, I just updated my pull request fixing some bugs and supporting more jQuery versions.

you can test the new plugin from this jsfiddle: (working fine from jQuery 1.6.4 to 2.1.0)
http://jsfiddle.net/6u6nrmq0/

and compare the results with the current version:
http://jsfiddle.net/n6jgbztz/

@skovhus
Copy link

skovhus commented Nov 17, 2014

Look interesting... But some unit test could be awesome. 👍

Mathieu Ghaleb added 2 commits April 8, 2015 15:45
@dcramer
Copy link
Member

dcramer commented Jul 25, 2015

No isolation, but basic functional test for one func in fa86837

@dcramer dcramer merged commit 8f9930e into getsentry:master Jul 25, 2015
@dcramer
Copy link
Member

dcramer commented Jul 25, 2015

Not entirely sure of how best to improve the tests.

Ideally we'd:

  • get the $.ajax tests in
  • ensure that deferred behaves the same as before (not just that it captures errors)

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.

5 participants