rusackas commented on code in PR #42602:
URL: https://github.com/apache/superset/pull/42602#discussion_r3707301844
##########
superset-frontend/packages/superset-core/src/theme/Theme.tsx:
##########
@@ -214,9 +230,42 @@ export class Theme {
emotionCache: createCache({ key: 'superset' }),
});
- this.updateProviders = (theme, antdConfig, emotionCache) => {
- setThemeState({ theme, antdConfig, emotionCache });
- };
+ // Register (and, on unmount, deregister) this provider instance's own
+ // listener rather than assigning a single shared callback on every
+ // render, so every concurrently mounted provider for this Theme
+ // instance receives updates, not just the last one to render.
+ //
+ // Use useLayoutEffect (not useEffect) so registration happens in the
+ // same commit phase as any layout effect elsewhere that might call
+ // setConfig/toggleDarkMode on this instance during mount (e.g. the
+ // docs site's dark-mode sync in StorybookWrapper.jsx, which reads the
+ // toggle and pushes it onto the singleton via a layout effect of its
+ // own). Layout effects run bottom-up, so a listener registered here
+ // (this component is nested inside that caller) is guaranteed to be
+ // in place before an ancestor's layout effect can fire and notify it.
+ // If this were a passive effect instead, an ancestor's layout effect
+ // could call toggleDarkMode before this listener exists, dropping that
+ // notification, and the provider would render stale until a later
+ // toggle.
+ // eslint-disable-next-line react-hooks/rules-of-hooks
+ React.useLayoutEffect(() => {
Review Comment:
Fair point in the abstract, but I don't think it bites today. The only thing
that mutates the singleton in a layout effect is ThemeSync, and it's always an
ancestor of SupersetThemeProvider, never a descendant, nothing inside a demo's
children calls toggleDarkMode/setConfig on mount. Agree useSyncExternalStore
would close this class of race generically if we ever grow a descendant that
does that. Worth revisiting then rather than reworking this now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
