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

Unmounting a tree of components #47

Closed
kruczy opened this issue Feb 12, 2016 · 12 comments
Closed

Unmounting a tree of components #47

kruczy opened this issue Feb 12, 2016 · 12 comments
Assignees
Labels
Milestone

Comments

@kruczy
Copy link
Contributor

kruczy commented Feb 12, 2016

Hi,
Today I came about an issue that if I remove a tree of components/elements only the top one will get the componentWillUnmount/componentDidUnmount called

I guess this could happen because components get cached and reused later, tho because of this nested components cannot cleanup timers or dispose of unnecessary resources

maybe https://github.com/developit/preact/blob/5b2ba167cb4e9acc7c0d265358ad7bf056eb46e2/src/vdom/component.js#L235
should be deepHook instead of just hook?

Thanks for any help!

@developit
Copy link
Member

@kruczy excellent catch, I don't know how I missed this! It'll need something more complicated than deepHook since there could be grandchild components. I'll get a fix out right away.

@developit developit added the bug label Feb 12, 2016
developit added a commit that referenced this issue Feb 12, 2016
@developit
Copy link
Member

@kruczy Sorry, I must have been tired earlier - you were exactly right, it just needed to be a deepHook().
I just released 3.3.0, which includes this fix (JSFiddle example).

@kruczy
Copy link
Contributor Author

kruczy commented Feb 15, 2016

Hey @developit, been testing this today and it seems deepHook is not enough as it only calls one level down, components that are below that do not get the hook fired. maybe deepHook should be walking the tree all the way down?

@developit
Copy link
Member

@kruczy yeah deepHook only handles high-order components. You're seeing grandchild components not unmount?

Component
    div
        Component   - this one?

@kruczy
Copy link
Contributor Author

kruczy commented Feb 15, 2016

From my observation that would work
but if you create a deeper hierarchy it does not

in my case the tree is 6 components deep I unmount the whole thing at once but unmount hook seems to be called only on the first 2 levels

@kruczy
Copy link
Contributor Author

kruczy commented Feb 15, 2016

I modified your example to show the problem http://jsfiddle.net/orgkorjc/10/

Notice that C only logs on the first iteration and then does not log mounts or unmounts

@developit
Copy link
Member

Perfect, that is immensely useful. Will get a fix out for this ASAP.

@developit developit reopened this Feb 15, 2016
@developit
Copy link
Member

@kruczy This was possibly the biggest rabbit hole I've encountered since adding support for High-Order components to Preact, but I think I've got it solved.

The solution actually necessitated a slight change in how Preact mounts and re-mounts Components, but I think it's a better long-term approach: components will now own their rendered content. Unmounting a component now retains the most recently rendered snapshot (this.base) in-tact with the component when reclaimed by the recycler. Previously, unmounting a component would retain the old DOM within the render parent, and diff it against whatever replaced the component (including if that happened to be null, which would be a recursive recycling op - not very efficient!

With the new semantics for component mount/unmount, the second time you mount a component, it simply diffs against the most recent snapshot. I think this will help with use-cases like routers, where the DOM rendered by different child components of a router is expected to change by somewhere near 100%. It will incur a small overhead when swapping two components that render nearly identical DOM structures, but this can be avoided by serving that use-case using Pure Functions, which always diff against the previous render, even if the component/function is a different reference.

Apologies for the long-winded and perhaps slightly unrelated comment - just useful for my own brain and to document this somewhere! Expect a release soon that fixes your issue.

@kruczy
Copy link
Contributor Author

kruczy commented Feb 16, 2016

Thanks for support and explanation, always great to have a responsive maintainer 👍

@developit developit added this to the 4.0 milestone Feb 22, 2016
@developit developit self-assigned this Feb 22, 2016
@developit
Copy link
Member

@kruczy This is fixed in 4.0.0 (currently released under a beta tag, npm i preact@beta).

@kruczy
Copy link
Contributor Author

kruczy commented Feb 24, 2016

thanks, this is fixed: http://jsfiddle.net/z43dp0ok/1/

@developit
Copy link
Member

Thanks again for that JSFiddle demo - it's so much easier to diagnose issues with minimal examples. 👍

slmgc added a commit to ekorzun/react-template that referenced this issue Jul 11, 2016
marvinhagemeister added a commit that referenced this issue Mar 2, 2019
Validate the referential equality of the args to componentDidUpdate
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants