Permalink
Browse files

fix(jqLite): data should store data only on Element and Document nodes

This is what jQuery does by default: https://github.com/jquery/jquery/blob/c18c6229c84cd2f0c9fe9f6fc3749e2c93608cc7/src/data/accepts.js#L16

We don't need to set data on text/comment nodes internally and if we don't
allow setting data on these nodes, we don't need to worry about cleaning
it up.

BREAKING CHANGE: previously it was possible to set jqLite data on Text/Comment
nodes, but now that is allowed only on Element and Document nodes just like in
jQuery. We don't expect that app code actually depends on this accidental feature.
  • Loading branch information...
IgorMinar authored and rodyhaddad committed Jun 4, 2014
1 parent ea9a130 commit a196c8bca82a28c08896d31f1863cf4ecd11401c
Showing with 4 additions and 1 deletion.
  1. +4 −1 src/jqLite.js
View
@@ -316,7 +316,10 @@ function jqLiteData(element, key, value) {
}
if (isSetter) {
data[key] = value;
// set data only on Elements and Documents
if (element.nodeType === 1 || element.nodeType === 9) {
data[key] = value;
}
} else {
if (keyDefined) {
if (isSimpleGetter) {

6 comments on commit a196c8b

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Jun 21, 2014

Member

@rodyhaddad & @IgorMinar - Isn't this check too late? It will still create the jqLiteExpandoStore object and attach that to the jqCache on line 315

Member

petebacondarwin replied Jun 21, 2014

@rodyhaddad & @IgorMinar - Isn't this check too late? It will still create the jqLiteExpandoStore object and attach that to the jqCache on line 315

@jbedard

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard Jun 23, 2014

Contributor

This is the main reason for the leak part of #7913. Here's an example:

    it('should ignore data on non-element/document nodes', function() {
      var calcCacheSize = function() {
        var size = 0;
        forEach(jqLite.cache, function(item, key) { size++; });
        return size;
      };

      var c = jqLite([
        document.createTextNode('text'),
        document.createComment('comment'),
        document.createDocumentFragment()
      ]);

      //Methods that depend on data
      c.data('should', 'not be set');
      c.on('click', noop);
      c.scope();

      expect(calcCacheSize()).toBe(0);
    });
Contributor

jbedard replied Jun 23, 2014

This is the main reason for the leak part of #7913. Here's an example:

    it('should ignore data on non-element/document nodes', function() {
      var calcCacheSize = function() {
        var size = 0;
        forEach(jqLite.cache, function(item, key) { size++; });
        return size;
      };

      var c = jqLite([
        document.createTextNode('text'),
        document.createComment('comment'),
        document.createDocumentFragment()
      ]);

      //Methods that depend on data
      c.data('should', 'not be set');
      c.on('click', noop);
      c.scope();

      expect(calcCacheSize()).toBe(0);
    });
@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Jun 23, 2014

Member

@jbedard - I don't think this is actually the cause of our leak in #7913 - I have a fix in #7942 for that
But I am concerned that this could allow other memory leaks.

Member

petebacondarwin replied Jun 23, 2014

@jbedard - I don't think this is actually the cause of our leak in #7913 - I have a fix in #7942 for that
But I am concerned that this could allow other memory leaks.

@jbedard

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard Jun 23, 2014

Contributor

@petebacondarwin Either way, shouldn't this nodeType check be in a common location such as jqLiteExpandoStore? I don't think #7942 will fix the test case I have above because .data still creates the expando store (but doesn't put the data in it).

Contributor

jbedard replied Jun 23, 2014

@petebacondarwin Either way, shouldn't this nodeType check be in a common location such as jqLiteExpandoStore? I don't think #7942 will fix the test case I have above because .data still creates the expando store (but doesn't put the data in it).

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Jun 23, 2014

Member

@jbedard - yes I agree I think there is a problem here.
@IgorMinar - what you do think?

Member

petebacondarwin replied Jun 23, 2014

@jbedard - yes I agree I think there is a problem here.
@IgorMinar - what you do think?

@IgorMinar

This comment has been minimized.

Show comment
Hide comment
@IgorMinar

IgorMinar Jun 24, 2014

Member

yup. you are right. we still create empty objects.

Member

IgorMinar replied Jun 24, 2014

yup. you are right. we still create empty objects.

Please sign in to comment.