refactor(jqLite): stop patching individual jQuery methods #7288
Conversation
|
Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.
If you need to make changes to your pull request, you can update the commit with Thanks again for your help! |
|
From your description, it sounds reasonable to me. I'm not necessarily a jQuery expert, though, it would be good to hear from one of them to see if it's the right approach, or if there might be something better like hooks that we could patch into. |
|
Well, I'm a member of the jQuery Core team. ;-) We had some discussions if we'll be able to remove For the record, this is the relevant jQuery UI patch: https://github.com/jquery/jquery-ui/blob/83b3d67175f16057844e5719bcda56eeb74560f3/ui/widget.js#L26-36 |
|
I see, I guess I won't cc dmethvin then! But yeah it seems like a sane thing to do. How far back into jQuery version-history would this extend? |
Very far. ;) Even jQuery 1.4.2 (& I guess many older but I didn't check) has this method but Angular is compatible with 1.7.0+ because of the switch from .bind/.unbind to .on/.off. |
|
@caitp Since this introduces a (minor, but still) breaking change, should I wait for LGTMs from some other team members before I commit it to master? |
|
You should wait for LGTMs from others regardless! I've asked people to take a look at it, but I'm not sure who would be the best to review it. |
|
I've changed the |
|
@caitp I know @IgorMinar was keen on making Angular jQuery 2-compatible quickly so maybe he should be cc'd to stuff like this. |
|
@mzgol @IgorMinar is out most of the week unfortunately, but if he does pop in I'll mention it I guess |
|
lgtm |
Currently Angular monkey-patches a few jQuery methods that remove elements
from the DOM. Since methods like .remove() have multiple signatures
that can change what's actually removed, Angular needs to carefully
repeat them in its patching or it can break apps using jQuery correctly.
Such a strategy is also not future-safe.
Instead of patching individual methods on the prototype, it's better to
hook into jQuery.cleanData and trigger custom events there. This should be
safe as e.g. jQuery UI needs it and uses it. It'll also be future-safe.
The only drawback is that $destroy is no longer triggered when using $detach
but:
1. Angular doesn't use this method, jqLite doesn't implement it.
2. Detached elements can be re-attached keeping all their events & data
so it makes sense that $destroy is not triggered on them.
3. The approach from this commit is so much safer that any issues with
.detach() working differently are outweighed by the robustness of the code.
BREAKING CHANGE: the $destroy event is no longer triggered when using the
jQuery detach() method. If you want to destroy Angular data attached to the
element, use remove().
Currently Angular monkey-patches a few jQuery methods that remove elements
from the DOM. Since methods like .remove() have multiple signatures
that can change what's actually removed, Angular needs to carefully
repeat them in its patching or it can break apps using jQuery correctly.
Such a strategy is also not future-safe.
Instead of patching individual methods on the prototype, it's better to
hook into jQuery.cleanData and trigger custom events there. This should be
safe as e.g. jQuery UI needs it and uses it. It'll also be future-safe.
The only drawback is that $destroy is no longer triggered when using $detach
but:
so it makes sense that $destroy is not triggered on them.
.detach() working differently are outweighed by the robustness of the code.
BREAKING CHANGE: the $destroy event is no longer triggered when using the
jQuery detach() method. If you want to destroy Angular data attached to the
element, use remove().
cc @caitp