-
Notifications
You must be signed in to change notification settings - Fork 41
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
Flush styles synchronously #72
Conversation
@@ -22,7 +22,8 @@ | |||
"release": "np --no-2fa" | |||
}, | |||
"peerDependencies": { | |||
"react": ">=16.8" | |||
"react": ">=16.8", | |||
"react-dom": ">=16.8" |
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're going to need this. Idk if this deserves a major/minor bump. Since this package technically only works with react-dom anyway.
"@types/react": "^16.9.19", | ||
"@types/react-dom": "^16.9.5", | ||
"@types/react": "^17.0.3", | ||
"@types/react-dom": "^17.0.3", |
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.
These expose flushSync
. However, the method existed in 16.8 too (which is your minimal version). It just wasn't in the types.
isExpanded ? {} : collapsedStyles | ||
); | ||
const mergeStyles = useCallback((newStyles: {}): void => { |
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 removed useCallback because I don't see the point in having it here.
Thanks!! |
@gaearon By invoking I know it is a side effect, but I'm curious |
This fixes a problem I've observed in this package with React 18 alpha. (Sorry, I don't have a quick repro case.)
We're setting styles via state. In React 18 alpha, state updates outside of event handlers are batched, too. So we can't 100% rely on reading layout in a rAF (like this package does with
height
) unless we force the styles to be flushed as soon as possible.An alternative solution would be to skip React entirely and set styles in the DOM directly. That seems like a bigger change so I didn't go there.
I've verified this change fixes an issue in my project when clicking expand/collapse too fast.