1368659 - [eslint] Write a rule that detects DOM manipulation in a loop over that DOM

Status: NEW

(Developer Infrastructure :: Lint and Formatting, enhancement, P3)

Description

[:Gijs (away/busy until Mar 10)]

Consider:

for (let x of y.children) {
  z.appendChild(x);
}

This will only append half of y's children into z, because the DOM children and childNodes collections are live to DOM changes, and we're changing the set of children/childNodes mid-iteration because appendChild moves the nodes elsewhere.

It would be nice if eslint errored in cases where we're using a for...of loop where both of these conditions hold:
1) the bound variable (ie x in the above example) for the loop is created out of a .children or .childNodes or other live DOM collection (at least one of getElementsBy* or querySelector* is live, and the other isn't - I always forget which)
2) the bound variable is passed as the first argument to insertBefore, appendChild or removeChild, or <bound>.remove() is called.

Comment 1

[Mark Banner (:standard8)]

Gijs, can you suggest some valid options, and maybe point to somewhere we've done this incorrectly before?

Comment 2

[:Gijs (away/busy until Mar 10)]

(In reply to Mark Banner (:standard8) (afk 11 - 20 Aug) from comment #1)
> Gijs, can you suggest some valid options, and maybe point to somewhere we've
> done this incorrectly before?

Reader mode is the infamous example (well, back in 2015 or whatever) of where we did this incorrectly with a traditional for loop:

for (var i = 0; i < parent.childNodes.length; i++) {
  if (parent.childNodes[i].someCondition) {
    somewhereElse.appendChild(parent.childNodes[i]);
  }
}

would only check and re-append half the things.

Alternatives include:

0) use a non-live thing (see comment #0 about querySelector* vs. getElements*, but this doesn't work for foo.children / foo.childNodes)
1) making a non-live array first, e.g. using Array.from();
2) iterating in reverse
3) iterating with a while loop if you're removing everything anyway: while (node.firstChild) node.firstChild.remove();
4) iterating with a while loop over nodes, ie:

let kid = node.firstChild;
while (kid) {
  let thisKid = kid;
  kid = kid.nextElementSibling;
  doSomethingWith(thisKid);
}

5) separate the "find all the things to append / reinsert / remove" and the "do the append / reinsert / remove" steps by collecting a specific set of elements in a set/array first, and then bulk-removing/appending/inserting those.

Does that help?

Comment 3

[Mark Banner (:standard8)]

Yes thanks.