Recreate styles when theme changes

[noratarano]

Add the ability to recalculate styles if the theme has changed. This
allows for the style function of a component already having one to
recreate its style function from the updated theme, and will ensure
that every component is styled with the same theme.

to @majapw @ljharb @goatslacker

[majapw]

FYI @gabergg @lencioni

This seems pretty reasonable to me! :)

[majapw]

so in theory, if we wanted to lock down a component and prevent retheming entirely, we could pass in true for this prop? If so, that'd be good to document in the README

[noratarano]

Good call!

[gabergg]

I probably have a slight preference for leaving this option out on this pass. I don't imagine many people will be intentionally registering new themes and counting on the fact that components that have already been rendered won't update.

[majapw]

I wonder if this is something we want to hold in the withStyles HOC or whether we'd want it as a flag in the ThemedStyleSheet.

This is probably more performant here @martinwin although maybe a bool in the ThemedStyleSheet would be cleaner. idk.

[noratarano]

I agree. I had that thought too. I figured that ThemedStyleSheet shouldn't define the concept of changed this way necessarily (is it when a theme is registered, when the theme changes with !==, or when an invalidation function is called explicitly?), so I left it out of there. Or maybe it should? I can see it either way.
Thoughts? @majapw

Edit: Ignore bad code.

The alternative would look like with the naming TBD: ```jsx // ThemedStyleSheet let themeChanged;

function registerTheme(theme) {
themeChanged = theme !== styleTheme;
styleTheme = theme;
}

function hasThemeChanged() {
return themeChanged;
}

```jsx
// withStyles
maybeCreateStyles() {
  const recreateStyles = !lockTheme && ThemedStyleSheet.hasThemeChanged();
  // ...
  if (isRTL && (!styleDefRTL || recreateStyles)) {
  // ...
}

[noratarano]

Actually this may be cleaner ^

[majapw]

Hmm we'd have to have a way to prevent future recreates after this as well, e.g. once we've built the styleSheet with createRTL/createLTR we could update the themeChanged variable again (trickily, we'd have to ... have a flag for each direction, no?)

[noratarano]

Wait no, because if the theme is registered only once, then styles would be recreated every time.

[noratarano]

@majapw, @gabergg PTAL, how's this?

[gabergg]

Per our offline conversation, this global toggle will create an issue if multiple components have been rendered before the new theme is registered. The first component will recreate its styles, flipping the global toggle, and the rest of the components will fail to recreate their styles. So, it likely makes sense to bring this logic back into withStyles, so it's handled with a component-level flag.

[noratarano]

OK! Thanks @gabergg and @majapw for your insightful review. I put the theme tracking back in the HOC. This way, theme changes will be managed by each component and we prevent inter-component side effects. I also updated the tests to catch all of the cases we discussed. What do you think about this now?

[majapw]

This seems legit! I'm wondering if we can somehow separate out the if/else of the LTR vs RTL world into one function that takes an argument.

[gabergg]

LGTM!