fix(jqLite): make jqLite invoke jqLite.cleanData as a method #15846
Conversation
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
src/jqLite.js
Outdated
| @@ -309,13 +303,11 @@ function jqLiteClone(element) { | |||
| } | |||
|
|
|||
| function jqLiteDealoc(element, onlyDescendants) { | |||
| if (!onlyDescendants) jqLiteRemoveData(element); | |||
| if (!onlyDescendants && element.nodeType === NODE_TYPE_ELEMENT) jqLite.cleanData([element]); | |||
There was a problem hiding this comment.
Are we sure this doesn't break some usecases or tests?
| @@ -309,13 +303,10 @@ function jqLiteClone(element) { | |||
| } | |||
|
|
|||
| function jqLiteDealoc(element, onlyDescendants) { | |||
| if (!onlyDescendants) jqLiteRemoveData(element); | |||
| if (!onlyDescendants && jqLiteAcceptsData(element)) jqLite.cleanData([element]); | |||
There was a problem hiding this comment.
@gkalpak Let me elaborate why it shouldn't break anything now. The only situation where we would fire jqLiteRemoveData(element) before and not fire jqLite.cleanData([element]) here now is when jqLiteAcceptsData(element) is false. But if you then look at jqLiteRemoveData source, it changes anything only if element.ng339 is defined. This property is only set to something non-undefined inside jqLiteExpandoStore and only if the second passed argument is truthy. That, in turn, happens in two places only: one is inside jqLiteData, the other one is in jqLite#on. In both cases this setter is guarded by the jqLiteAcceptsData(element) being truthy which contradicts our assumptions.
Therefore, there should be no observable differences to external code here.
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
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
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Fix.
What is the current behavior? (You can also link to an open issue here)
jqLite.cleanDatais not used directly so it can't be monkey-patched like its jQuery equivalent.What is the new behavior (if this is a feature change)?
The opposite.
Does this PR introduce a breaking change?
I don't think so.
Please check if the PR fulfills these requirements
Docs have been added / updated (for bug fixes / features)Other information:
This is a followup to #8695.