New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mutation observers: Avoid adding disconnected elements #161

Merged
merged 4 commits into from Jun 1, 2018

Conversation

Projects
None yet
2 participants
@javan
Contributor

javan commented Jun 1, 2018

This change works around a somewhat unexpected MutationObserver behavior: When a node is removed from an observed tree and then another node is added to it synchronously, the observer will include that added node in its next batch of mutation records. Example: https://jsfiddle.net/javan/acxd6v1x/

So, if you remove an element from the DOM and then add a data-controller element to it, Stimulus will match the added element and install a new controller. Because the element is already disconnected from the DOM, Stimulus will never unmatch and disconnect() its controller. AKA a memory leak. Example: https://jsfiddle.net/javan/vqht4n5s/

@javan javan requested a review from sstephenson Jun 1, 2018

@sstephenson

This comment has been minimized.

Contributor

sstephenson commented Jun 1, 2018

Do we need an isConnected polyfill?

@javan

This comment has been minimized.

Contributor

javan commented Jun 1, 2018

The isConnected test is an optimization.

if (element.isConnected != this.element.isConnected) {
return false
} else {
return this.element.contains(element)
}

Browsers that do support it will return a boolean, and if there's a mismatch we can skip the slightly more expensive element.contains() call in the else branch. Browsers that don't support will return undefined and jump straight to the else branch.

@javan

This comment has been minimized.

Contributor

javan commented Jun 1, 2018

A polyfill might look something like this, which would end up being even more work:

if (!("isConnected" in Node.prototype)) {
  Object.defineProperty(Node.prototype, "isConnected", {
    get() {
      return document.documentElement.contains(this)
    }
  })
}

(not that it really matters)

@javan javan merged commit 6946174 into master Jun 1, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@javan javan deleted the fix-matching-synchronous-disconnects branch Jun 1, 2018

@javan javan added this to the 1.1 milestone Jun 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment