-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[IndexTable] Improve reflow perf #6840
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
Conversation
|
/snapit |
size-limit report 📦
|
69eabd5 to
cc3375c
Compare
|
/snapit |
|
🫰✨ Thanks @AndrewMusgrave! Your snapshot has been published to npm. Test the snapshot by updating your yarn add @shopify/[email protected] |
59f6eb6 to
b02e8f6
Compare
|
/snapit |
|
🫰✨ Thanks @AndrewMusgrave! Your snapshot has been published to npm. Test the snapshot by updating your yarn add @shopify/[email protected] |
| export interface TableHeadingRect { | ||
| offsetWidth: number; | ||
| offsetLeft: number; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget to hide whitespace!
| SIXTY_FPS, | ||
| {leading: true, trailing: true, maxWait: SIXTY_FPS}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When these values are omitted debounce will fallback to RAF.
Sixty fps is aprox 16.7ms which is how often RAF will fire. Rather than estimate when the frame will be, let's use it!
b02e8f6 to
767d63e
Compare
| } | ||
| const handleCanScrollRight = useCallback( | ||
| () => | ||
| debounce(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This'll default to raf
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| const handleResize = useCallback( | ||
| debounce(() => { | ||
| if (position !== 0) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only need to calculate the offset once, not for every checkbox
767d63e to
1f95bf5
Compare
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| const handleCanScrollRight = useCallback( | ||
| debounce(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This'll default to raf
| if (!checkboxNode.current) return; | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| const handleResize = useCallback( | ||
| debounce(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This'll default to raf
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| const handleResize = useCallback( | ||
| debounce(() => { | ||
| if (position !== 0 || !checkboxNode.current) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only need to set the offset once, not for every checkbox!
| }); | ||
| setGetBoundingClientRect(200); | ||
|
|
||
| act(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
act is needed since component state changes outside trigger (which automatically uses act)
|
/snapit |
|
🫰✨ Thanks @AndrewMusgrave! Your snapshot has been published to npm. Test the snapshot by updating your yarn add @shopify/[email protected] |
df45c80 to
7a5fde9
Compare
chloerice
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a huge improvement, thanks Andrew!! 🚀 🚀 🚀
polaris-react/src/components/IndexTable/components/Checkbox/tests/Checkbox.test.tsx
Outdated
Show resolved
Hide resolved
| ) { | ||
| return; | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused by this, why not pass an inline function that returns the debounce? I see it done in both ways in this file, what's the difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah I get it, the difference between this executing immediately vs calling it directly.
sethomas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kk awesome, I did 10 tests each for before and after for the Recalculate Style:
Before average: 63.28ms
After average: 41.61ms
I wasn't able to see massive gains or numbers like you mentioned, but I could also have been doing something wrong.
Let's take a look durning our pairing tomorrow. 60 is still double what ive been seeing so it'll be interesting to see where it's coming from 🤔 |
|
We resolved it! Nice! Issues that I was doing:
Thanks for looking with me about what the discrepancies were. |
mrcthms
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice perf gains!
512540b to
d9b5e41
Compare
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @shopify/[email protected] ### Minor Changes - [#6890](#6890) [`267e1a9bd`](267e1a9) Thanks [@alex-page](https://github.com/alex-page)! - Add deprecated scss API to stylelint-polaris and use regex tests in the config ## @shopify/[email protected] ### Patch Changes - [#6840](#6840) [`90f325460`](90f3254) Thanks [@AndrewMusgrave](https://github.com/AndrewMusgrave)! - Removed additional reflows from IndexTable ## [email protected] ### Minor Changes - [#7074](#7074) [`ddfacd855`](ddfacd8) Thanks [@alex-page](https://github.com/alex-page)! - Replace data/\*.json files with build time .cache/site.json - [#7082](#7082) [`8ebb9fc3c`](8ebb9fc) Thanks [@selenehinkley](https://github.com/selenehinkley)! - Large edit of /contributing - [#7087](#7087) [`43ea7e5d5`](43ea7e5) Thanks [@alex-page](https://github.com/alex-page)! - Add automation to generate .cache/nav.json ### Patch Changes - [#7037](#7037) [`7dafdee00`](7dafdee) Thanks [@alex-page](https://github.com/alex-page)! - Move header logic to the API and out of next.config.js - [#7075](#7075) [`5933fc547`](5933fc5) Thanks [@alex-page](https://github.com/alex-page)! - Run gen-assets on build of website - Updated dependencies \[[`90f325460`](90f3254)]: - @shopify/[email protected] ## [email protected] ### Patch Changes - Updated dependencies \[[`90f325460`](90f3254)]: - @shopify/[email protected] Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
WHY are these changes introduced?
Fixes https://github.com/Shopify/web/issues/70102
A lot of time is spent recalculating styles, that becomes more significant the more complex the page is.
WHAT is this pull request doing?
Using RFA to delay checking node attributes, to avoid reflows.
Screenies
The self time was reduced from ~29ms to ~9ms
How to 🎩
Bottom-uptab forrecalculate stylesPlayground code below
Copy-paste this code in
playground/Playground.tsx:Demo 🎩 gif
🎩 checklist
README.mdwith documentation changes