Skip to content

Commit

Permalink
Do not expose internal children array
Browse files Browse the repository at this point in the history
Whenever Cheerio inserts a node as a child of a some other node, it
explicitly removes it from its previous `children` array. This both
avoids memory leaks and more faithfully matches the behavior of the W3C
DOM, where a node may only be a child of one other node.

When a user creates an array of node objects from a string using
`$.parseHTML`, she most likely intends to insert the contents into some
previously-existing DOM structure. If supplied with the `children` array
directly, future insertion operations will modify the contents of that
array. This is both unintuitive from an API design perspective and
inconsistent with jQuery's behavior.

To avoid silently changing the contents of arrays given to users, update
`$.parseHTML` to return a shallow copy of the `children` structure.
  • Loading branch information
jugglinmike committed Feb 21, 2015
1 parent ab1e4a0 commit b876f09
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 1 deletion.
7 changes: 6 additions & 1 deletion lib/static.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,12 @@ exports.parseHTML = function(data, context, keepScripts) {
parsed('script').remove();
}

return parsed.root()[0].children;
// The `children` array is used by Cheerio internally to group elements that
// share the same parents. When nodes created through `parseHTML` are
// inserted into previously-existing DOM structures, they will be removed
// from the `children` array. The results of `parseHTML` should remain
// constant across these operations, so a shallow copy should be returned.
return parsed.root()[0].children.slice();
};

/**
Expand Down
8 changes: 8 additions & 0 deletions test/api/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,14 @@ describe('cheerio', function() {
expect(cheerio.parseHTML('<#if><tr><p>This is a test.</p></tr><#/if>') || true).to.be.ok();
});

it('(text) : should return an array that is not effected by DOM manipulation methods', function() {
var $ = cheerio.load('<div>');
var elems = $.parseHTML('<b></b><i></i>');

$('div').append(elems);

expect(elems).to.have.length(2);
});
});

describe('.contains', function() {
Expand Down

0 comments on commit b876f09

Please sign in to comment.