TableView reduce renders and onLoadMore#3529
Conversation
|
Build successful! 🎉 |
snowystinger
force-pushed
the
fix-table-renders
branch
from
1922103 to
d49a2bb
Compare
|
Build successful! 🎉 |
|
Build successful! 🎉 |
LFDanLu
left a comment
LFDanLu
left a comment
There was a problem hiding this comment.
Tested in Chrome, FF, Safari, and Chrome android, LGTM. Just one request for a comment update
| // choosing | ||
| // - state.contentSize.height > 0 && state.contentSize.height // if height reduces (delete the page) then we want to load more | ||
| // - state.virtualizer.contentSize.height // not an entry in dependency array, so if we remove the state.contentSize, this won't work if we removed the dependency we aren't using | ||
| // - ref for initial render | ||
| // - ref to store content size from virtualizer and remove dependency array <- |
There was a problem hiding this comment.
nit: this comment makes sense with the PR description's context, but I feel like it doesn't without that context. Mind just mentioning in this comment that these were potential entries in a dep array that didn't work out hence why we run it every render instead?
|
Build successful! 🎉 |
|
Build successful! 🎉 |
| }, | ||
| "dependencies": { | ||
| "@babel/runtime": "^7.6.2", | ||
| "@react-aria/utils": "^3.13.3", |
There was a problem hiding this comment.
This is an aria dependency in a stately package
There was a problem hiding this comment.
ugh.... fails SSR Test, i'll look into it more tomorrow
snowystinger
force-pushed
the
fix-table-renders
branch
from
cf3f261 to
4bd44be
Compare
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
| act(() => {jest.runAllTimers();}); | ||
|
|
||
| expect(onLoadMore).toHaveBeenCalledTimes(3); | ||
| expect(onLoadMore).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
I could see how the first one is not scrolling far enough to cause a onLoadMore to call, but shouldn't the second one? I'd want to see expect(onLoadMore).toHaveBeenCalledTimes(0); after the first two scrolls. Should there be a test after this that does call onLoadMore twice?
|
closing in favor of #3672 |
5 participants
Closes
In collaboration with @devongovett
This addresses some extra renders and extra layout effect calls that were noticed in the resizing PR
useTableColumnResizeState now has internal state, where, if no prop "onResize", is provided, it'll use its own state to cause a re-render. However, if onResize is provided, then it's assumed that the component calling the hook will trigger a re-render in some fashion. This could be from a layout or their own state management.
We make use of the onResize in our TableView because we have a layout and virtualizer, so we invoke the re-render with a relayoutNow.
Previously useTableColumnResizeState called a state update, and then in an effect later, relayoutNow was called. Now we just go directly to the relayoutNow, skipping the first render that the setState triggered.
This brought up a case where we still had two renders though, as setTableWidth causes a relayoutNow and setVisibleRect causes a relayout. The order we had them in meant that relayoutNow was called and then another relayout was scheduled after a raf. Instead, we can coalesce those into one render by scheduling the relayout and then immediately doing the relayoutNow which will handle everything in the scheduled update immediately as well.
This brought up yet another issue where we had some tests that accounted for way too many calls to onLoadMore, which we've always felt was off anyways. Our number of renders was now correct, but our layout effect was being called with a stale value, resulting in onLoadMore being called when it shouldn't be. We considered a few different ways of running the layout effect, however, we found our common methods of choosing a dependency array to be insufficient. I've outlined what we considered in a comment in the code.
Performance testing
Initial Load:
no difference, the build on main spent 459ms in initial TableView, the build on my branch spent 450ms by comparison
Resize:
build on main each resize took on average 95ms
build on my branch each resize took on average 48ms
in the same span of about 1second, main managed 9 frames vs my branch did 14 frames
main also averaged 2-3 recalculate style calls to the browser in between each resize, while my branch averaged 1
one interesting thing, that i don’t quite know how it fits in is that main ran each resize under onGlobalMessage and barely ran anything under onPointerMove
meanwhile, my branch ran most of everything under onPointerMove and had a much much smaller onGlobalMessage call
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: