Skip to content
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

javan
Copy link
Contributor

@javan 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 June 1, 2018 18:56
@sstephenson
Copy link
Contributor

Do we need an isConnected polyfill?

@javan
Copy link
Contributor Author

javan commented Jun 1, 2018

The isConnected test is an optimization.
https://github.com/stimulusjs/stimulus/blob/d3ea33716e2315ae3a1c0189e61bb1e136eccd82/packages/%40stimulus/mutation-observers/src/element_observer.ts#L132-L136
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
Copy link
Contributor Author

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
@javan javan deleted the fix-matching-synchronous-disconnects branch June 1, 2018 19:53
@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
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants