fix(jqLite): make removeData() not remove event handlers #16512
Conversation
| function isEmptyObject(obj) { | ||
| var name; | ||
|
|
||
| for (name in obj) { |
There was a problem hiding this comment.
Do we really care about inherited properties here?
There was a problem hiding this comment.
That's taken from jQuery data handling:
https://github.com/jquery/jquery/blob/3.3.1/src/event.js#L290-L293
https://github.com/jquery/jquery/blob/3.3.1/src/data/Data.js#L142-L153
| @@ -615,6 +633,7 @@ forEach({ | |||
| cleanData: function jqLiteCleanData(nodes) { | |||
| for (var i = 0, ii = nodes.length; i < ii; i++) { | |||
| jqLiteRemoveData(nodes[i]); | |||
| jqLiteOff(nodes[i]); | |||
There was a problem hiding this comment.
AFAICT, $destroy handlers will not be removed, which means that the expandoStore will never be completely empty and removed.
There was a problem hiding this comment.
This should be handled in the cleanData patch that used to be applied to jQuery only and that I moved to apply to jqLite as well.
test/jqLiteSpec.js
Outdated
| }); | ||
|
|
||
|
|
||
| it('should allow to set data after removeData() with event handlers present', function() { |
There was a problem hiding this comment.
What about "with event handlers not present"?
Good point. Although this method is still not officially documented in jQuery although it seems we'd like to change it.
The confusion comes from the fact jqLite code refes to bare DOM nodes as |
|
BTW, should this also be changd to |
Perhaps. |
This commit removes the resetting of `angular.element.cache` in some tests; this was desynchronizing jqLite.cache & the local jqCache variable and since some parts of the code use one of them and some the other one, it was breaking JQLite._data. `angular.element.cache` doesn't even exist when jQuery 2+ is used. Closes #16515 Refs #16512
This commit removes the resetting of `angular.element.cache` in some tests; this was desynchronizing jqLite.cache & the local jqCache variable and since some parts of the code use one of them and some the other one, it was breaking JQLite._data. `angular.element.cache` doesn't even exist when jQuery 2+ is used. Closes #16515 Refs #16512
test/jqLiteSpec.js
Outdated
| }); | ||
| jqLite.cleanData(selected); | ||
|
|
||
| browserTrigger(a, 'click'); |
test/jqLiteSpec.js
Outdated
|
|
||
| jqLite.cleanData(selected); | ||
|
|
||
| browserTrigger(a, 'click'); |
test/jqLiteSpec.js
Outdated
| expect(jqLite(c).data('prop')).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('should remove user data & event handlers on cleanData()', function() { |
There was a problem hiding this comment.
This description belongs to the test above and vice versa.
|
PR updated once again. |
This change aligns jqLite with jQuery. Fixes angular#15869 BREAKING CHANGE: Before this commit `removeData()` invoked on an element removed its event handlers as well. If you want to trigger a full cleanup of an element, change: ```js elem.removeData(); ``` to: ```js angular.element.cleanData(elem); ``` In most cases, though, cleaning up after an element is supposed to be done only when it's removed from the DOM as well; in such cases the following: ```js elem.remove(); ``` will remove event handlers as well.
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix but a breaking change as well.
What is the current behavior? (You can also link to an open issue here)
removeData()invoked on an element removes its event handlers as well.What is the new behavior (if this is a feature change)?
removeData()removes just user-setdata, not internal data storage used for event handling. This aligns jqLite with jQuery.Does this PR introduce a breaking change?
Yes.
Please check if the PR fulfills these requirements
Fix/Feature: Docs have been added/updated(the current behavior is not documented)Other information:
Fixes #15869