ThemeProvider context missing issues, should we support a default context?

[i832513]

In SuccessFactors, we have this issue where sometimes ThemeProvider gets added twice into the context tree causing non-fatal React errors to show up.

console.js:213 Warning: Detected multiple renderers concurrently rendering the same context provider. This is currently unsupported.
[stack trace]

These errors can be ignored - but it can be frustrating because in our app-toolkit wrappers are supposed to add the ThemeProvider for us, but sometimes it doesn't. And in the cases that it doesn't add the ThemeProvider - the app just crashes spectacularly with a blank page and errors in the console because the <Text> component says it needs a ThemeProvider. To avoid those fatal issues in the middle-layers we'll add the <ThemeProvider> again which fixes the problem when the wrapper didn't add the provider but adds this warning to the logs when the wrapper does add it. I suppose we could create a new Provider called <ConditionalThemeProvider> or something that just adds the ThemeProvider wrapper only when it's not already there - but that seems silly.

Anyway - I realize that we need to track down these issues and make sure the app-toolkit is consistently adding the ThemeProvider so that our apps can rely on that always being there and then remove the Provider from the middle layers; however, the whole ThemeProvider concept as a whole seems unnecessary - since in our case we never pass any value or additional information to the ThemeProvider - we just need to add an empty <ThemeProvider> and then things start to work.

In theory, shouldn't there be some sort of default Context that would do the same thing?

I would understand it better if we were actually setting something on the <ThemeProvider> that would make it so that only one place in the app could be setting the theme, but since we are not it seems more like an unnecessary Framework quirk that we need to add that Provider layer.

[i832513]

In my opinion there are really a few options to resolve this type of issue:

1. Update the ui5-webcomponents-react layer so that we don't need to add the ThemeProvider at all - we could then remove it from each layer (app-toolkit + middle-app layer)
2. Fix the app-toolkit layer such that we are able to rely on it always adding the ThemeProvider precisely once, and remove any unnecessary duplicate theme provider layers in the middle app-layer.
3. Remove the ThemeProvider additions from the upper app-toolkit layers, and ensure that the app-layers will always define this ThemeProvider at the top most Application Component layer.
4. Add a kind of ConditionalThemeProvider that will add the ThemeProvider but only conditionally if the Theme context is not already existing, then we can add these freely to the middle-layer without triggering the react warning messages.

I've ranked these in my preference. I opened this topic because my hope was that we would be able to remove the ThemeProvider layer - which has been a kind of thorn in my side.

NOTE: In the past, many components like <Text> would work fine without a <ThemeProvider> despite the fact that the documentation states clearly you always need to have it. This creates a problem where many times we'll have Jest Test cases where we forgot to add the ThemeProvider and everything works perfectly fine for a long time. Then all of a sudden a component (like <Text> for example) adds a dependency to a hook that triggers a fatal exception in a new version (e.g. "1.27.0" is such a version) where dozens of unit tests are now failing. So to fix those dozens of unit tests I need to go to each one and add the <ThemeProvider> to the render(<ThemeProvider>....</ThemeProvider>) to ensure that the snapshots generate something rather than crashing with no content.

Perhaps now that 1.27 enforces the ThemeProvider on even the most simple component like <Text> we will always write our unit tests with a ThemeProvider -- but you can see how I wish we didn't have to always add this annoying ThemeProvider to every unit test.

[MarcusNotheis]

Hi @i832513,

the ThemeProvider is actually providing some contexts which are hidden from the consumer, and at the end of the day the name ThemeProvider is not really describing what this component is actually doing:

1. It injects some globally applied styles and CSS variables depending on the current theme
2. It renders the a provider for the ModalsContext which enables the imperative method like useShowMessageBox()
3. It renders a provider for all translations (this is also the reason why the Text now needs a ThemeProvider).

We're currently reworking how we are handling our global app states while making sure they are still working with Microfrontends by leveraging the useSyncExternalStore hook. With that hook we're also able to fix the multiple renderers concurrently rendering the same context provider warning. We've already moved the styles and i18n to the external store and will now look into the modals provider.

With these changes it should then be possible to run (and test) the individual components without the ThemeProvider, but for full functionality (e.g. supporting language changes) the ThemeProvider is still required in the app.