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

Fix TableView resize observer loop limit #5432

Merged
merged 4 commits into from Nov 22, 2023

Conversation

snowystinger
Copy link
Member

@snowystinger snowystinger commented Nov 17, 2023

Closes #1924

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

@snowystinger
Copy link
Member Author

Closed #5367 in favor of this change

let raf = useRef<ReturnType<typeof requestAnimationFrame> | null>();
let onResize = () => {
if (isOldReact) {
raf.current ??= requestAnimationFrame(() => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

React 16 and 17 don't batch like 18. We instead emulate that (to a degree) by waiting for a raf before submitting our state changes

We also tried unstable_batchupdates, which solved most of the issue. However, when the scrollbar appeared, we got the dreaded loop limit exceeded. This is likely because two updates were coming in faster than a single raf. First, the rect size change due to the resize of the window. Second, a rect size change due to the scroll bar appearing.

@rspbot
Copy link

rspbot commented Nov 17, 2023

@snowystinger snowystinger marked this pull request as ready for review November 17, 2023 04:47
@snowystinger snowystinger changed the title Fix resize observer loop limit Fix TableView resize observer loop limit Nov 17, 2023
@rspbot
Copy link

rspbot commented Nov 22, 2023

@rspbot
Copy link

rspbot commented Nov 22, 2023

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }

@snowystinger snowystinger merged commit 75417bb into main Nov 22, 2023
26 checks passed
@snowystinger snowystinger deleted the fix-scrollview-resize-observer branch November 22, 2023 02:47
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.

useResizeObserver not preventing Resize Observer errors in some situations
4 participants