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

(fix) - ensure correct unmount #1687

Merged
merged 4 commits into from
Jun 10, 2019
Merged

(fix) - ensure correct unmount #1687

merged 4 commits into from
Jun 10, 2019

Conversation

JoviDeCroock
Copy link
Member

Fixes: #1632
Adds a test for the wrong dom scenario (isn't fixed yet) but I aim at doing this in the future

@coveralls
Copy link

coveralls commented Jun 6, 2019

Coverage Status

Coverage remained the same at 99.778% when pulling b973c40 on fix/unmount into 9ead713 on master.

@JoviDeCroock
Copy link
Member Author

Hmm, with the changes from @andrewiggins this change does not seem to be needed... Anyone sees a scenario where we should at it for safety?

@marvinhagemeister
Copy link
Member

That's awesome! Even if the original issue is shady fixed by other changes in master I'd love to merge the Test cases in this PR. They seem to valuable not to have 👍

@JoviDeCroock
Copy link
Member Author

@marvinhagemeister would you like me to revert the change and just PR with the tests?

Copy link
Member

@andrewiggins andrewiggins left a comment

Choose a reason for hiding this comment

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

I say let's check this PR as is. After working on my last PRs, I don't think_lastDomChild should be used as proxy for determining if something is Fragment. Since all Components are Fragments now, we should directly check the type (i.e. typeof vnode.type === 'function') or the presence of _component for behavior like this.

@andrewiggins andrewiggins merged commit 3499b05 into master Jun 10, 2019
@andrewiggins andrewiggins deleted the fix/unmount branch June 10, 2019 20:05
@andrewiggins
Copy link
Member

I went ahead and merged this cuz I'm building on top of it for my next PR

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.

DOM elements are orphaned when rendered through a wrapping component
4 participants