developit / undom

🍩 1kb minimally viable DOM Document implementation
https://npm.im/undom
MIT License
668 stars 25 forks source link

Problem with spies #35

Open mindplay-dk opened 3 years ago

mindplay-dk commented 3 years ago

I'm trying to spy on removeChild in a test - I have a parent node being modified like this:

    let removals = 0;

    let removeChild = parent.removeChild.bind(parent);

    parent.removeChild = (child) => {
      removals += 1;
      return removeChild(child);
    };

To my surprise, it was counting way too many calls to removeChild.

It turns out, several of these methods are being called internally, for brevity.

The real DOM does not call it's own public methods, afaik? If it needs to remove children for other reasons than calls to removeChild, it does not internally call the method, it just removed them.

How would you feel about moving the internally reused methods to private methods?

mindplay-dk commented 3 years ago

To illustrate what I'm proposing:

image

For now, I'm using a modified copy of this library in my test-suite, and just made this minimal change so I can finish. If you'd like, I'd be happy to go over the rest and submit a PR to make sure no methods are calling any public methods.

(I supposed these methods could be #private as well, but I don't know how you feel about using private class fields with only two thirds of browsers supporting them? Though I suppose microbundle takes care of all that...)

developit commented 3 years ago

@mindplay-dk I think we can just add internal helper methods that live off the class, like remove(parent, child).

mindplay-dk commented 3 years ago

"Off the class", so not methods, just functions? Like this?

function createEnvironment() {
    // ...
    function remove(parent, child) {
        splice(parent.childNodes, child, false, true);
        return child;
    }
    // ...
    class Node {
        // ...
        removeChild(child) {
            return remove(this, child);
        }
        remove() {
            if (this.parentNode) remove(this.parentNode, this);
        }
    }
    // ...
}