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

Improve null placeholder DOM placement #2355

Merged
merged 9 commits into from
Feb 17, 2020

Conversation

andrewiggins
Copy link
Member

Previously if a Component went from rendering DOM to rendering null, our child diffing algorithm would have to re-place all children after the DOM that was removed. This PR improves the diffing algorithm to detect when this has happened and continue diffing from the removed DOM node's sibling.

Also reverts a change I made to a test in #2352 that invalidated the test.

Fixes #2350

andrewiggins and others added 6 commits February 15, 2020 18:14
A test is failing cuz when trying to find the sibling dom in `getDomSibling`, a parent is in the middle of diffing and it's `_children` prop is set to it's render result, which is not an array.

The `_parent` property is shared between oldVNode and newVNode in this situation so when traversing up oldVNode `_parent`, you reach a parent's newVNode. In other words, in `renderComponent` , `/* oldVNode */ assign({}, vnode)._children[0]._parent == /* newVNode */ vnode`
* fix failing test by defaulting to empty array

* use correct import

* apply different fix
@github-actions
Copy link

github-actions bot commented Feb 17, 2020

Size Change: +93 B (0%)

Total Size: 38.5 kB

Filename Size Change
dist/preact.js 3.73 kB +23 B (0%)
dist/preact.min.js 3.73 kB +23 B (0%)
dist/preact.module.js 3.75 kB +21 B (0%)
dist/preact.umd.js 3.79 kB +26 B (0%)
ℹ️ View Unchanged
Filename Size Change
compat/dist/compat.js 3 kB 0 B
compat/dist/compat.module.js 3.03 kB 0 B
compat/dist/compat.umd.js 3.05 kB 0 B
debug/dist/debug.js 3.08 kB 0 B
debug/dist/debug.module.js 3.07 kB 0 B
debug/dist/debug.umd.js 3.15 kB 0 B
devtools/dist/devtools.js 175 B 0 B
devtools/dist/devtools.module.js 185 B 0 B
devtools/dist/devtools.umd.js 252 B 0 B
hooks/dist/hooks.js 1.05 kB 0 B
hooks/dist/hooks.module.js 1.08 kB 0 B
hooks/dist/hooks.umd.js 1.13 kB 0 B
test-utils/dist/testUtils.js 390 B 0 B
test-utils/dist/testUtils.module.js 392 B 0 B
test-utils/dist/testUtils.umd.js 469 B 0 B

compressed-size-action

@coveralls
Copy link

coveralls commented Feb 17, 2020

Coverage Status

Coverage increased (+0.0004%) to 99.792% when pulling 4dadad5 on fix/null-placeholder-sibling-placement into 0024fb9 on master.

Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

Super excited about this! Great work all around 😍🙌💯

src/diff/children.js Outdated Show resolved Hide resolved
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.

iframe reloads when parent's sibling returns null
4 participants