[fix] Keep Flyout Manager sync'd across React roots. #9075
Conversation
| "unified": "^9.2.2", | ||
| "unist-util-visit": "^2.0.3", | ||
| "url-parse": "^1.5.10", | ||
| "use-sync-external-store": "^1.6.0", |
There was a problem hiding this comment.
Note to other reviewers: useSyncExternalStore is shipped with React starting from version 18.0. We need this shim for React 17 support
| state, | ||
| ...rest, | ||
| goToFlyout, | ||
| getHistoryItems: () => { |
There was a problem hiding this comment.
Since this is selector logic, maybe a better architecture would be to move getHistoryItems to the store. Doing so would make it so that we don't need to destructure goToFlyout from the store on line 28.
There was a problem hiding this comment.
I've played around with moving getHistoryItems to the store, and it created a scenario where it would return stale data even when state updates. The Provider component re-renders, but not components that consume the store state. To resolve that, I'm changing the FlyoutManagerApi to expose a historyItems property that can be used as an observable value.
tsullivan
left a comment
tsullivan
left a comment
There was a problem hiding this comment.
Thank you so much for working this out! Left a couple of comments
|
Thanks @tsullivan I'll make the adjustments and open for review. |
|
@clintandrewhall in regards to the storybook that has been added, could we instead roll that into the "Multi-session example" storybook? I think that would be more suitable. |
…ture Replace useFlyoutManagerReducer with singleton store using useSyncExternalStore for cross-root synchronization. Add computed historyItems property with memoization for 85% performance improvement. Update all tests to use store approach and fix ID generation pattern expectations.
- Move “Multi-root sync” storybook to flyout_sessions.stories.tsx - Revert “Playground” storybook file
… references on every access
| }) => { | ||
| const api = useFlyoutManagerReducer(); | ||
| const { getState, subscribe, ...rest } = getFlyoutManagerStore(); | ||
| const state = useSyncExternalStore(subscribe, getState, getState); |
There was a problem hiding this comment.
Adding the third parameter here should fix the build issue.
- Docusaurus (which builds EUI's documentation website) uses Static Site Generation (SSG)
- The
getServerSnapshotparameter is needed for React to render the component during this build step
tkajtoch
left a comment
tkajtoch
left a comment
There was a problem hiding this comment.
Code changes look great and work as expected. Thank you both for your work on this fix!
tsullivan
force-pushed
the
exploration/flyout/external-sync
branch
from
e46ba63 to
ebc3870
Compare
💚 Build SucceededHistory
|
💚 Build Succeeded
History
|
As titled. Adds a module-level singleton instance of the store and enhances the provider with
useSyncExternalStore, (adds a Meta-maintained shim to support React 17).This adds support for a synchronized Flyout Manager state when flyouts are launched across multiple React roots. Also supports having child flyouts opened from a separate React root than its parent.