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

Fragment diff issue #2551

Merged
merged 21 commits into from Jun 18, 2020
Merged

Fragment diff issue #2551

merged 21 commits into from Jun 18, 2020

Conversation

Copy link
Member

@JoviDeCroock JoviDeCroock commented May 24, 2020

Fixes #2527
The issue at hand: when updating components with children who are strictly equal or implement SCU they could have reordered due to the parent rendering. This works when the children are just DOM-nodes but becomes an issue when these children are Fragments or nested Fragments. We need to update _nextDom so the parents diffChildren can know where to continue after it has diffed this child, so for instance when we see the following:



const Child = () => (
  <Fragment>
    <div>1</div>
    <div>2</div>
  </Fragment>
);

const Parent = () => (
  <div>
    <Child />
    <div />
  </div>
);



The _nextDom property should signal to diffChildren(Parent) that after the diff of children.Child it has to place the <div /> after the last dom-property wrapped by the Fragment in this case <div>2</div>, but what happens if our Child bails out of render, and returns props.children? These children could be reordered, … by the Parent component. This means that our dom-reference could be stale.

When we aren’t bailing out of render the placeChild function handles this by tracking _nextDom and reordering everything however when we bail out there is no such logic. For this we abstracted out the placeChild logic and reused it in SCU/strict-equality bailout, this deeply traverses the DOM when it sees that it isn’t equal and does the placement. This is still an optimized path as compared to non-scu/non-bailout.

I guess we could introduce better bailouts but up to now I'm not entirely sure how yet.

@github-actions
Copy link

@github-actions github-actions bot commented May 24, 2020

Size Change: +324 B (0%)

Total Size: 40 kB

Filename Size Change
dist/preact.js 3.96 kB +77 B (1%)
dist/preact.min.js 3.98 kB +83 B (2%)
dist/preact.module.js 3.97 kB +82 B (2%)
dist/preact.umd.js 4.01 kB +82 B (2%)
ℹ️ View Unchanged
Filename Size Change
compat/dist/compat.js 3.2 kB 0 B
compat/dist/compat.module.js 3.23 kB 0 B
compat/dist/compat.umd.js 3.25 kB 0 B
debug/dist/debug.js 2.99 kB 0 B
debug/dist/debug.module.js 2.98 kB 0 B
debug/dist/debug.umd.js 3.08 kB 0 B
devtools/dist/devtools.js 184 B 0 B
devtools/dist/devtools.module.js 195 B 0 B
devtools/dist/devtools.umd.js 260 B 0 B
hooks/dist/hooks.js 1.09 kB 0 B
hooks/dist/hooks.module.js 1.11 kB 0 B
hooks/dist/hooks.umd.js 1.17 kB 0 B
test-utils/dist/testUtils.js 437 B 0 B
test-utils/dist/testUtils.module.js 439 B 0 B
test-utils/dist/testUtils.umd.js 515 B 0 B

compressed-size-action

@JoviDeCroock JoviDeCroock marked this pull request as ready for review Jun 12, 2020
src/diff/index.js Outdated Show resolved Hide resolved
src/diff/index.js Outdated Show resolved Hide resolved
src/diff/index.js Outdated Show resolved Hide resolved
@JoviDeCroock JoviDeCroock requested a review from developit Jun 15, 2020
@andrewiggins
Copy link

@andrewiggins andrewiggins commented Jun 16, 2020

Nice description - thanks for writing that up!

I think much the size savings come from converting reorderChildren to a function declaration. The results of compressed-size-action are the same for remove const in favor of let and rename more const to let (I'm looking at the "Size Differences" section which I think compares that size of the commit to master).

Should we revert 8fca6c9 if it doesn't actually change the size? Just thinking we shouldn't give contributors the wrong idea about let/const unless it does affect size.

@JoviDeCroock
Copy link
Author

@JoviDeCroock JoviDeCroock commented Jun 16, 2020

Yes, we should probably rever that one

Copy link
Member

@andrewiggins andrewiggins left a comment

Thanks for pushing this through! Was definitely an interesting fix :)

test/browser/fragments.test.js Outdated Show resolved Hide resolved
src/diff/index.js Outdated Show resolved Hide resolved
JoviDeCroock and others added 2 commits Jun 17, 2020
Co-authored-by: Andre Wiggins <459878+andrewiggins@users.noreply.github.com>
Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

This is amazing! Really impressed with both of you @JoviDeCroock and @andrewiggins on narrowing this down 🙌

@JoviDeCroock JoviDeCroock merged commit 8825bf1 into master Jun 18, 2020
7 checks passed
@JoviDeCroock JoviDeCroock deleted the fragment-diff branch Jun 18, 2020
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.

4 participants