Fixed nested themes not being republished on outer theme changes #363

Andarist · 2017-12-21T00:31:41Z

What:

This fixes a bug when having nested ThemeProviders and updating outer theme the change is not reaching the StyledComponent

<ThemeProvider theme={{}}>
	<ThemeProvider theme={{}}>
		<StyledComponent>
	</ThemeProvider>
</ThemeProvider>

Why:

This is a bug fix.

How:
Using publish in the nested ThemeProviders subscriptions.

Checklist:

Tests
Code complete

Repro provided by @mitchellhamilton - https://codesandbox.io/s/6j42qx5z23

codecov-io · 2017-12-21T00:34:54Z

Codecov Report

Merging #363 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #363   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          10     10           
  Lines         178    179    +1     
  Branches       50     50           
=====================================
+ Hits          178    179    +1

Impacted Files	Coverage Δ
src/theme-provider.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb6de3a...a904c67. Read the comment docs.

kentcdodds

This looks fine (and awesome, thanks!), but I'm least comfortable with the theme code in this project. I didn't develop it myself, I rarely use it, and I'm not certain I understand all the use cases. So I'd really like someone who uses glamorous's ThemeProvider to give this a review as well. Thanks!

kentcdodds · 2017-12-21T18:06:28Z

src/theme-provider.js

@@ -53,13 +58,12 @@ class ThemeProvider extends React.Component {
    // set broadcast state by merging outer theme with own
    if (this.context[CHANNEL]) {
      this.setOuterTheme(this.context[CHANNEL].getState())
-      this.broadcast.setState(this.getTheme())


Can you explain why this was removed? It seems unrelated.

publishTheme method is doing a setState and setOuterTheme uses now publishTheme under the hood

I think this is the real fix, and publishTheme is only incidentally changed.

This line was setting the state with no argument, which defaults to the props.theme here

glamorous/src/theme-provider.js

Line 21 in cb6de3a

const theme = passedTheme || this.props.theme

, meaning it's not setting the theme to the next theme it's just resetting it to the theme it already has.

A lot of the theming work was done by @vesparny, so if I'm wrong hopefully he can correct me. 😄

Yeah, this part of defaulting to this.props.theme is a making a control flow a little bit iffy but I left this part untouched. Personally I would prefer explicit arguments, so that publishTheme would always have to receive getTheme's result as argument which in turn would always be called with this.props.theme if necessary.

Also I've noticed a different small issue too - broadcast is initialized as:

broadcast = brcast(this.props.theme)

but that gives it untrue initialState in case of nested ThemeProvider. I think creation of the broadcast should be delayed.

but that gives it untrue initialState in case of nested ThemeProvider. I think creation of the broadcast should be delayed.

I'm pretty sure I follow, but to be clear, are you suggesting moving it to something like componentWillMount? With this PR, the theme that was (possibly) merged with the outer theme is now going to be set properly inside componentWillMount(), so that untrue initial state won't really ever be in effect.

Did I follow your point correctly? If not any clarification would be great. 🙌

Yeah, that was exactly my point :)

Cool, thanks! It makes sense to me. Maybe you want to do that as a follow-up PR?

Sure thing, would prefer to create it once we settle on this one to avoid potential conflicts

Thanks for the discussion here!

Andarist · 2017-12-21T18:52:41Z

@kentcdodds do u have any other reviewer in mind? The chane is pretty straightforward and the codebase is tested so it shouldnt break anything ;)

paulmolluzzo · 2017-12-21T19:07:19Z

src/theme-provider.js

    }
  }

  componentWillReceiveProps(nextProps) {
    if (this.props.theme !== nextProps.theme) {
-      this.broadcast.setState(this.getTheme(nextProps.theme))
+      this.publishTheme(nextProps.theme)


Pardon my ignorance, but how has this made an effectual change? publishTheme is calling broadcast.setState(this.getTheme(nextProps.theme)) which is was was here before. It's only been moved to a separate method.

oh, this part is actually not a fix of any sorts, i just have extracted it to separate method so it would be easier to reuse it in setOuterTheme

OK, thanks for clarifying.

paulmolluzzo

LGTM.

Frankly, I haven't used nested ThemeProviders that much so if @vesparny or @goodmind have time to review that'd be great.

Whoops, forgot to say thank you @Andarist!

kentcdodds

Super! Thank you for your help!

kentcdodds · 2017-12-22T05:33:54Z

src/theme-provider.js

@@ -53,13 +58,12 @@ class ThemeProvider extends React.Component {
    // set broadcast state by merging outer theme with own
    if (this.context[CHANNEL]) {
      this.setOuterTheme(this.context[CHANNEL].getState())
-      this.broadcast.setState(this.getTheme())


Thanks for the discussion here!

Fixed nested themes not being republished on outer theme changes

a904c67

Andarist force-pushed the fix/nested-theme-providers branch from f96a56a to a904c67 Compare December 21, 2017 11:44

kentcdodds reviewed Dec 21, 2017

View reviewed changes

paulmolluzzo reviewed Dec 21, 2017

View reviewed changes

paulmolluzzo approved these changes Dec 21, 2017

View reviewed changes

kentcdodds approved these changes Dec 22, 2017

View reviewed changes

kentcdodds merged commit 8cd90bd into paypal:master Dec 22, 2017

Andarist deleted the fix/nested-theme-providers branch December 22, 2017 11:03

Andarist mentioned this pull request Dec 22, 2017

Delay instantation of brcast in ThemeProvider so it can be inited wit… #365

Merged

4 tasks

dependencies bot mentioned this pull request Dec 22, 2017

Update 19 dependencies from npm storybookjs/storybook#2540

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed nested themes not being republished on outer theme changes #363

Fixed nested themes not being republished on outer theme changes #363

Andarist commented Dec 21, 2017 •

edited

Loading

codecov-io commented Dec 21, 2017 •

edited

Loading

kentcdodds left a comment

kentcdodds Dec 21, 2017

Andarist Dec 21, 2017

paulmolluzzo Dec 21, 2017 •

edited

Loading

Andarist Dec 21, 2017

paulmolluzzo Dec 21, 2017

Andarist Dec 21, 2017

paulmolluzzo Dec 21, 2017

Andarist Dec 21, 2017

kentcdodds Dec 22, 2017

Andarist commented Dec 21, 2017

paulmolluzzo Dec 21, 2017

Andarist Dec 21, 2017

paulmolluzzo Dec 21, 2017

paulmolluzzo left a comment •

edited

Loading

kentcdodds left a comment

kentcdodds Dec 22, 2017

Fixed nested themes not being republished on outer theme changes #363

Fixed nested themes not being republished on outer theme changes #363

Conversation

Andarist commented Dec 21, 2017 • edited Loading

codecov-io commented Dec 21, 2017 • edited Loading

Codecov Report

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmolluzzo Dec 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarist commented Dec 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmolluzzo left a comment • edited Loading

Choose a reason for hiding this comment

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarist commented Dec 21, 2017 •

edited

Loading

codecov-io commented Dec 21, 2017 •

edited

Loading

paulmolluzzo Dec 21, 2017 •

edited

Loading

paulmolluzzo left a comment •

edited

Loading