fix(jqLite.cleanData): TypeError: Cannot read property 'events' of undefined

[alextkachman]

I'm submitting a ...

• [x ] bug report

Current behavior:
TypeError happens when data is cleaned for OBJECT element. OBJECT does not support data

Expected / new behavior:
no TypeError expected

Minimal reproduction of the problem with instructions:
OBJECT element removed by because of ng-if instruction

        <div ng-if="shownGraphic.tag === 'multimedia' && shownGraphic.multimediaObject.multimediaType === 'swf'" style="width: 100%; height: 100%;">
            <object style="width: 100%; height: 100%;"><embed style="width: 100%; height: 100%;" embed-src="{{shownFigureView.url()}}" /></object>
        </div>

AngularJS version:
Angular 1.7.2
JQuery 1.10.2

Browser:
Presumably All

Anything else:
TypeError: Cannot read property 'events' of undefined
at Function.jqLite.cleanData (angular.js:2051)
at replaceWith (angular.js:10704)
at applyDirectivesToNode (angular.js:9768)
at compileNodes (angular.js:9293)
at compileNodes (angular.js:9305)
at compileNodes (angular.js:9305)
at compileNodes (angular.js:9305)
at compile (angular.js:9174)
at Object.link (angular.js:28966)
at angular.js:1364

[gkalpak]

This is a jQuery 1.x issue. Not sure which version it was fixed in, but jQuery 3.x (as well as AngularJS' built-in jqLite) always return an object.

[alextkachman]

This is so wrong. It for years worked correctly elem._data('events') and recently was changed to incorrect elem._data().events with no obvious for me reason

[gkalpak]

Hm...OK if we can fix this is a non-breaking way, then there is little reason not to, I guess 😃

It was changed in b7d396b.
@mgol, any objection against reverting jqLite._data(elem).events to jqLite._data(elem, 'events')? Is this going to affect the behavior with latest jQuery?

[mgol]

@alextkachman AngularJS hasn't supported jQuery 1.x since version 1.3 if I remember correctly. This version was released almost 4 years ago. Your configuration has been unsupported for a long time so something may randomly break even in a patch release; we can't prevent that from happening as we don't run unit tests with jQuery 1.x. You should move to a supported version of jQuery, i.e. 2.1 or newer.

@gkalpak We can't just revert the change as jqLite doesn't support the two-param _data signature and the linked commit changed the cleanData patch to be performed on jqLite in addition to jQuery. That's why I changed this line.

Fixing this issue requires to do one of the following:

1. fork the logic inside of the patch for jQuery & jqLite
2. add support for the two-param _data to jqLite
3. add a check for jQuery._data(elem) being defined before accessing its events property

I think what happens is jQuery 1.x uses jQuery.cleanData inside jQuery#remove internally where it removes the cache entry for the removed node before calling jQuery.cleanData, making jQuery._data(elem) return undefined inside of our patched jQuery.cleanData. In all other cases, even jQuery 1.x should be falling back to an empty object if there are no handlers attached.

[gkalpak]

Thx for the info, @mgol. Although jQuery 1.x is indeed not supported, I think option (3) is harmless (and straight forward). I'll submit a PR.

[mgol]

Yes, I forgot to add option 3 sounded the best to me as well as it's such a critical place and such an easy fix.

[alextkachman]

Thank you, @gkalpak and @mgol