Skip to content

(fix) - ensure correct unmount#1687

Merged
andrewiggins merged 4 commits into
masterfrom
fix/unmount
Jun 10, 2019
Merged

(fix) - ensure correct unmount#1687
andrewiggins merged 4 commits into
masterfrom
fix/unmount

Conversation

@JoviDeCroock

@JoviDeCroock JoviDeCroock commented Jun 6, 2019

Copy link
Copy Markdown
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

coveralls commented Jun 6, 2019 •
edited
Loading

Copy link
Copy Markdown

Coverage Status

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

@JoviDeCroock

JoviDeCroock commented Jun 10, 2019

Copy link
Copy Markdown
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

marvinhagemeister commented Jun 10, 2019

Copy link
Copy Markdown
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

JoviDeCroock commented Jun 10, 2019

Copy link
Copy Markdown
Member Author

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

andrewiggins
andrewiggins approved these changes Jun 10, 2019
View reviewed changes

@andrewiggins andrewiggins left a comment

Copy link
Copy Markdown
Member

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

andrewiggins commented Jun 10, 2019

Copy link
Copy Markdown
Member

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

@snyk-bot snyk-bot mentioned this pull request Oct 26, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andrewiggins andrewiggins andrewiggins approved these changes

@marvinhagemeister marvinhagemeister Awaiting requested review from marvinhagemeister

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

DOM elements are orphaned when rendered through a wrapping component

4 participants

@JoviDeCroock @coveralls @marvinhagemeister @andrewiggins