This repository was archived by the owner on Apr 12, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 27.4k
fix(jQuery): cooperate with other libraries monkey-patching jQuery.cleanData #8486
Merged
+79
−27
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4118,26 +4118,57 @@ describe('$compile', function() { | |
}); | ||
|
||
if (jQuery) { | ||
it('should clean up after a replaced element', inject(function ($compile) { | ||
var privateData, firstRepeatedElem; | ||
describe('cleaning up after a replaced element', function () { | ||
var $compile, xs; | ||
beforeEach(inject(function (_$compile_) { | ||
$compile = _$compile_; | ||
xs = [0, 1]; | ||
})); | ||
|
||
element = $compile('<div><div ng-repeat="x in xs">{{x}}</div></div>')($rootScope); | ||
function testCleanup() { | ||
var privateData, firstRepeatedElem; | ||
|
||
$rootScope.$apply('xs = [0,1]'); | ||
firstRepeatedElem = element.children('.ng-scope').eq(0); | ||
element = $compile('<div><div ng-repeat="x in xs">{{x}}</div></div>')($rootScope); | ||
|
||
expect(firstRepeatedElem.data('$scope')).toBeDefined(); | ||
privateData = jQuery._data(firstRepeatedElem[0]); | ||
expect(privateData.events).toBeDefined(); | ||
expect(privateData.events.$destroy).toBeDefined(); | ||
expect(privateData.events.$destroy[0]).toBeDefined(); | ||
$rootScope.$apply('xs = [' + xs + ']'); | ||
firstRepeatedElem = element.children('.ng-scope').eq(0); | ||
|
||
$rootScope.$apply('xs = null'); | ||
expect(firstRepeatedElem.data('$scope')).toBeDefined(); | ||
privateData = jQuery._data(firstRepeatedElem[0]); | ||
expect(privateData.events).toBeDefined(); | ||
expect(privateData.events.$destroy).toBeDefined(); | ||
expect(privateData.events.$destroy[0]).toBeDefined(); | ||
|
||
expect(firstRepeatedElem.data('$scope')).not.toBeDefined(); | ||
privateData = jQuery._data(firstRepeatedElem[0]); | ||
expect(privateData && privateData.events).not.toBeDefined(); | ||
})); | ||
$rootScope.$apply('xs = null'); | ||
|
||
expect(firstRepeatedElem.data('$scope')).not.toBeDefined(); | ||
privateData = jQuery._data(firstRepeatedElem[0]); | ||
expect(privateData && privateData.events).not.toBeDefined(); | ||
} | ||
|
||
it('should work without external libraries (except jQuery)', testCleanup); | ||
|
||
it('should work with another library patching jQuery.cleanData after Angular', function () { | ||
var cleanedCount = 0; | ||
var currentCleanData = jQuery.cleanData; | ||
jQuery.cleanData = function (elems) { | ||
cleanedCount += elems.length; | ||
// Don't return the output and expicitly pass only the first parameter | ||
// so that we're sure we're not relying on either of them. jQuery UI patch | ||
// behaves in this way. | ||
currentCleanData(elems); | ||
}; | ||
|
||
testCleanup(); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. don't you want to make some assertions on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops, I totally forgot about that. |
||
// The initial ng-repeat div is dumped after parsing hence we expect cleanData | ||
// count to be one larger than size of the iterated array. | ||
expect(cleanedCount).toBe(xs.length + 1); | ||
|
||
// Restore the previous jQuery.cleanData. | ||
jQuery.cleanData = currentCleanData; | ||
}); | ||
}); | ||
} | ||
|
||
|
||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IgorMinar I was wondering why this method was written in a way that is resistant do invoking multiple times. It doesn't seem to be done on the site but only in tests. Such a way of writing this cannot be resistant to other libs monkey-patching
cleanData
so I had to make sure it's invoked only once.Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is weird. somehow, sometime in the past we broke this. and I can't even see in git logs when or how it happened.
the way this used to work is that we would try to bind when
angular.js
is loaded and then again on bootstrap. That way we get to use jQuery even if it was loaded after angular. Obviously things don't work like this any more. It's kind of surprising that nobody has complained about this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. There's a larger question of how we want to proceed in various
script configurations. Currently if jQuery is included after Angular,
Angular will just not use it but otherwise it should work.
This PR is aimed at addressing the problem that when anything
monkey-patches jQuery.cleanData after Angular, the code stops working.
jQuery UI is given as an example (this one was actually reported) so one
can think why don't we just disallow it but: 1) it's hard to impossible to
provide a meaningful error message then; 2) it's about other libs as well.
So I think this change is needed but we're fine if we don't use jQuery if
it's loaded after Angular.
If we do want to bring back the support then it's going to be harder to
make it resilient. We could, though, set a flag on the jQuery object saying
if it was patched by us or not. We could then invoke this function as many
times we want and it would patch if and only if the particular jQuery
instance hasn't been patched by us yet.
Your call. :)
On Tue, Aug 12, 2014 at 6:24 PM, Igor Minar [email protected]
wrote:
Michał Gołębiowski