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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Portal children always being mounted #1781

Merged
merged 3 commits into from Jul 17, 2019
Merged

Conversation

marvinhagemeister
Copy link
Member

@marvinhagemeister marvinhagemeister commented Jul 17, 2019

This PR fixes an issue where the children of a Portal component would always be treated as new children, leading to incorrect componentDidMount calls. By switching to the raw preact render function we skip the dom from being destroyed in an update scenario.

The downside of this is that this definitely breaks the stacking scenario where multiple trees are rendered into the same dom node. I'm wondering if we should reject that case in general and push rethinking root handling after the X release.

Nevermind the stacking bit. Found a way to fix that too 馃帀

Fixes #1780

@coveralls
Copy link

coveralls commented Jul 17, 2019

Coverage Status

Coverage increased (+0.1%) to 99.688% when pulling 620509a on portals_destroyer into c4e2f37 on master.

Copy link
Member

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is clever! Great job

@marvinhagemeister marvinhagemeister merged commit c399a24 into master Jul 17, 2019
@steffenmllr
Copy link

Awesome fix @marvinhagemeister - this is the bug I just ran into :) Do you plan on making a 10.0.0-rc.1 release before the X release ?

@marvinhagemeister
Copy link
Member Author

@steffenmllr Good call, we should release another RC version soon. We need a bit more time on the docs site anyway. Th site is mostly ready and we're currently working on updating the content 馃帀

@steffenmllr
Copy link

steffenmllr commented Jul 26, 2019

@marvinhagemeister sounds great! Is there an easy way to install from git master?
I've tried npm install preactjs/preact#master but this fails because of the monorepo since it does not find preact/compat (I think)

@JoviDeCroock
Copy link
Member

if you npm install from a branch, then you will have to build. So you would have to include cd node_modules/preact && npm i && npm run build

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 this pull request may close these issues.

[10.0.0-rc.0] Portals DOM always destroyed on parent update
4 participants