Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

cleanData override breaks when jQuery UI is included after angular #8471

Closed
andyperlitch opened this issue Aug 4, 2014 · 5 comments · Fixed by #8486
Closed

cleanData override breaks when jQuery UI is included after angular #8471

andyperlitch opened this issue Aug 4, 2014 · 5 comments · Fixed by #8486

Comments

@andyperlitch
Copy link

@attilah sums this issue up very well in his gist, but here's the TLDR:

angular overrides jQuery.cleanData function, storing original in jQuery.cleanData.$$original. jQuery UI also overrides this method. So if you incidentally include jquery, then angular, then jquery UI (not farfetched, grunt bower-install does not guarantee a particular order), the $$original is no longer available and angular throws a undefined is not a function error when trying to call $$original.

@mgol
Copy link
Member

mgol commented Aug 4, 2014

The property is used because the bindJQuery function may be invoked twice and the $$original check is meant to prevent double wrapping. Since my recent commit upgrading to jQuery 2.1 (9e7cb3c) we also use .$$original in the $compile service.

We'll probably have to export it directly to the angular object as you proposed. I'll look into it.

Thanks for the report!

@mgol mgol self-assigned this Aug 4, 2014
@mgol mgol added this to the 1.3.0-beta.18 milestone Aug 4, 2014
@andyperlitch
Copy link
Author

no problem! thanks!

@mgol
Copy link
Member

mgol commented Aug 5, 2014

(thinking out loud)

I think that putting all other cleanData patches before the Angular's one will be required as we need to invoke the cleanData method in:

jQuery.cleanData.$$original([firstElementToRemove]);

and we need to invoke it without the Angular patch but with all other patches so that we don't break other libs. But if we wrapped first and jQuery UI wrapped second, we can't invoke the version patched only by jQuery UI since there's no such version available.

One way would be to extend the method with an optional additional parameter-flag indicating the Angular patch should be skipped but unfortunately jQuery UI invokes the patched method explicitely passing only the first parameter:
https://github.com/jquery/jquery-ui/blob/f7429edfe96d322cdec850f7207efba8125767a6/ui/widget.js#L26-L43
so the other one would be lost.

EDIT: or maybe I'll try to use a flag from outside the cleanData itself. Work in progress...

mgol added a commit to mgol/angular.js that referenced this issue Aug 5, 2014
…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
mgol added a commit to mgol/angular.js that referenced this issue Aug 5, 2014
…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
mgol added a commit to mgol/angular.js that referenced this issue Aug 12, 2014
…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
mgol added a commit to mgol/angular.js that referenced this issue Aug 12, 2014
…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
mgol added a commit to mgol/angular.js that referenced this issue Aug 14, 2014
…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
@mgol
Copy link
Member

mgol commented Aug 14, 2014

Fix landed.

@andyperlitch
Copy link
Author

Nice! Fantastic work, thanks a lot!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants