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

Flush styles synchronously #72

Merged
merged 1 commit into from
Oct 2, 2021
Merged

Flush styles synchronously #72

merged 1 commit into from
Oct 2, 2021

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Oct 1, 2021

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.

@@ -22,7 +22,8 @@
"release": "np --no-2fa"
},
"peerDependencies": {
"react": ">=16.8"
"react": ">=16.8",
"react-dom": ">=16.8"
Copy link
Contributor Author

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",
Copy link
Contributor Author

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 => {
Copy link
Contributor Author

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.

@roginfarrer roginfarrer merged commit 0fca69a into roginfarrer:main Oct 2, 2021
@roginfarrer
Copy link
Owner

Thanks!!

@cipherlogs
Copy link

@gaearon By invoking flushSync, the DOM is immediately updated but I wonder if all upcoming changes are typically pushed to the event loop. Rather than relying on flushSync, an alternative approach could involve using setTimeout (effect, 0). This ensures that the effect is pushed to the end of the event loop, thereby allowing it to run only after React has completed updating the DOM.

I know it is a side effect, but I'm curious

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.

3 participants