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

Atoms re-render whenever parent node updates (?) #421

Closed
joshfrench opened this issue Jun 17, 2016 · 3 comments · Fixed by #471
Closed

Atoms re-render whenever parent node updates (?) #421

joshfrench opened this issue Jun 17, 2016 · 3 comments · Fixed by #471
Assignees

Comments

@joshfrench
Copy link
Contributor

Once I've added an atom, its render() method is fired on any subsequent changes in the same section. For the Ember editor (and now the React version 😁) this means the ADD_ATOM_HOOK also fires again, and you end up with N+1 objects in componentAtoms which are never cleaned up.

Not sure if this is by accident or design, since cards don't exhibit this behavior (or more accurately can't, because you can't edit their node.) If it is intentional I'll open a bug in the Ember editor for cleaning up the orphaned componentAtoms.

atom

@bantic
Copy link
Collaborator

bantic commented Jun 21, 2016

Thanks for reporting this.
You're right, cards avoid this behavior because they never need to be re-rendered.
But sections do get re-rendered on each change.
Can you use the atom's onTeardown hook to handle any cleanup necessary? E.g.:

// atom.js
{
  render({env}) {
    env.onTeardown(() => {
      // do atom cleanup here
    });
  }
}

@joshfrench
Copy link
Contributor Author

onTeardown doesn't fire when the atom gets re-rendered as part of a section update, only when it's manually deleted/removed.

This is mostly a data sanitation thing. I'm not an Ember dev and my debugging skills there are limited, but I'm pretty sure every time an atom re-renders as part of a section update, the ADD_ATOM_HOOK adds another ref to it in componentAtoms and they're probably never getting GC'd. I didn't take quite the same path in the React version, but the same root cause still leaves me with a bunch of orphaned DOM nodes hanging out in memory (I haven't played around enough to confirm/deny whether those are getting GC'd eventually.)

@bantic
Copy link
Collaborator

bantic commented Jun 24, 2016

@joshfrench Yeah, you're absolutely correct. Data sanitation and also memory pressure concerns.
I'm going to make a parallel issue on the ember-mobiledoc-editor repo, too. The eventual goal here is minimal-as-possible rerenders, but that's a little ways off. :) I'll investigate and add some notes here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants