fix(jQuery): cooperate with other libraries monkey-patching jQuery.cleanData #8486
Conversation
| function bindJQuery() { | ||
| var originalCleanData; | ||
| if (bindJQueryFired) { | ||
| return; |
There was a problem hiding this comment.
@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.
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.
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 notifications@github.com
wrote:
In src/Angular.js:
function bindJQuery() {
- var originalCleanData;
- if (bindJQueryFired) {
- return;
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.—
Reply to this email directly or view it on GitHub
https://github.com/angular/angular.js/pull/8486/files#r16124157.
Michał Gołębiowski
|
This is best viewed with whitespace changes ignored due to required additional indenting: https://github.com/angular/angular.js/pull/8486/files?w=1 |
| jQuery.cleanData.$$original = originalCleanData; | ||
| jQuery.cleanData = (function (originalCleanData) { | ||
| return function(elems) { | ||
| if (!angular.$$skipDestroy) { |
There was a problem hiding this comment.
rather than putting this on angular it would be better to either put it on our cleanData fn or share it via internal variable.
There was a problem hiding this comment.
We can't put anything on cleanData because of the issue this PR is supposed to fix. :) We can't be sure if jQuery.cleanData is our monkey-patched version or one monkey-patched further by another library.
I can put it in an internal variable. I assume all variables defined in the top scope of this file are available in all others?
|
@IgorMinar Review comments addressed. |
|
Is the |
…eanData Some libraries (like jQuery UI) patch jQuery.cleanData as well. This commit makes Angular work correctly even if such external patching was done after the Angular one. Fixes angular#8471
…y UI So far it wasn't tested that Angular's logic for skipping it triggering the $destroy event on jQuery.cleanData in the replaceWith internal function works correctly when Angular is not the last one to patch the cleanData method (e.g. if jQuery UI does the patching later). This commits adds the relevant test. Ref angular#8486
…y UI So far it wasn't tested that Angular's logic for skipping it triggering the $destroy event on jQuery.cleanData in the replaceWith internal function works correctly when Angular is not the last one to patch the cleanData method (e.g. if jQuery UI does the patching later). This commits adds the relevant test. Ref angular#8486
…y UI So far it wasn't tested that Angular's logic for skipping it triggering the $destroy event on jQuery.cleanData in the replaceWith internal function works correctly when Angular is not the last one to patch the cleanData method (e.g. if jQuery UI does the patching later). This commits adds the relevant test. Ref angular#8486
…y UI So far it wasn't tested that Angular's logic for skipping it triggering the $destroy event on jQuery.cleanData in the replaceWith internal function works correctly when Angular is not the last one to patch the cleanData method (e.g. if jQuery UI does the patching later). This commits adds the relevant test. Ref angular#8486
…y UI So far it wasn't tested that Angular's logic for skipping it triggering the $destroy event on jQuery.cleanData in the replaceWith internal function works correctly when Angular is not the last one to patch the cleanData method (e.g. if jQuery UI does the patching later). This commits adds the relevant test. Ref angular#8486
…y UI So far it wasn't tested that Angular's logic for skipping it triggering the $destroy event on jQuery.cleanData in the replaceWith internal function works correctly when Angular is not the last one to patch the cleanData method (e.g. if jQuery UI does the patching later). This commits adds the relevant test. Ref angular#8486
…y UI So far it wasn't tested that Angular's logic for skipping it triggering the $destroy event on jQuery.cleanData in the replaceWith internal function works correctly when Angular is not the last one to patch the cleanData method (e.g. if jQuery UI does the patching later). This commits adds the relevant test. Ref #8486
The previous implementation of jqLite didn't use cleanData from the jqLite object but instead used a cached version which maede it impossible to monkey-patch jqLite.cleanData similarly to how you can do it in jQuery. This commit enables one of the tests so far run only with jQuery to run with jqLite as well. Ref angular#8486 Ref angular#8695
The previous implementation of jqLite didn't use cleanData from the jqLite object but instead used a cached version which maede it impossible to monkey-patch jqLite.cleanData similarly to how you can do it in jQuery. The cleanData method is not meant to be called directly by userland code; its purpose is mainly to be able to be monkey-patched; therefore, the previous implementation didn't make a lot of sense. This commit enables one of the tests so far run only with jQuery to run with jqLite as well. Ref angular#8486 Ref angular#8695
The previous implementation of jqLite didn't use cleanData from the jqLite object but instead used a cached version which maede it impossible to monkey-patch jqLite.cleanData similarly to how you can do it in jQuery. The cleanData method is not meant to be called directly by userland code; its purpose is mainly to be able to be monkey-patched; therefore, the previous implementation didn't make a lot of sense. This commit enables one of the tests so far run only with jQuery to run with jqLite as well. Ref #8486 Ref #8695 Closes #15846
…y UI So far it wasn't tested that Angular's logic for skipping it triggering the $destroy event on jQuery.cleanData in the replaceWith internal function works correctly when Angular is not the last one to patch the cleanData method (e.g. if jQuery UI does the patching later). This commits adds the relevant test. (cherry-picked from bf7685a) Ref #8486
The previous implementation of jqLite didn't use cleanData from the jqLite object but instead used a cached version which maede it impossible to monkey-patch jqLite.cleanData similarly to how you can do it in jQuery. The cleanData method is not meant to be called directly by userland code; its purpose is mainly to be able to be monkey-patched; therefore, the previous implementation didn't make a lot of sense. This commit enables one of the tests so far run only with jQuery to run with jqLite as well. (cherry-picked from bf5c2ee) Ref #8486 Ref #8695 Closes #15846
Some libraries (like jQuery UI) patch jQuery.cleanData as well. This commit
makes Angular work correctly even if such external patching was done after
the Angular one.
Fixes #8471
/cc @IgorMinar