Fix defaultProps/props theme theming by michalkvasnicak · Pull Request #253 · styled-components/styled-components

michalkvasnicak · 2016-11-24T16:32:27Z

Fixes #252
Fixes #247

mxstbr-bot · 2016-11-24T16:33:33Z

	3 Warnings
⚠️	changes to ThemeProvider.js might be semver major changes
⚠️	changes to StyledComponent.js might be semver major changes
⚠️	changes to StyledNativeComponent.js might be semver major changes

Generated by 🚫 danger

mxstbr

Amazing! Apart from the bot comments (add a CHANGELOG entry) I left a few comments inline, but other than those nitpicks this looks great! Thanks!

mxstbr · 2016-11-24T16:41:02Z

src/models/StyledComponent.js

+        }
+
+        // compare if there is any change
+        if (JSON.stringify(nextProps.theme) === JSON.stringify(this.state.theme)) {


Should this be a deep equality check instead of JSON.stringify? What's the difference?

@mxstbr I didn't want to install new dependency just because of this. But I am wondering if deep equality check is necessary. What do you think?

Isn't === enough?

Since the main difference between a deep equality check and the JSON.stringify method (which I find very smart btw, have never heard of that before) is that the deep equality check doesn't care about the ordering of the elements. (see here for more info)

How big is the a dependency that can do deep equality?

@mxstbr I don't know if there is need to do other equality check than ===. Because if we pass the same theme to generateAndInjectStyles() we should get the same result right?

I am looking on this https://github.com/substack/node-deep-equal.

Yeah I guess you're right. Let's bail out earlier if it's the exact same object, but otherwise do a stringify comparison:

if (nextProps.theme === this.state.theme || JSON.stringify(nextProps.theme) === JSON.stringify(this.state.theme)) {

This way we don't stringify if it's exact same object, which saves quite a bit of time, but then just stringify and compare deeply if they aren't.

Yeah this solution is better than introducing new dependency :)

mxstbr · 2016-11-24T16:42:46Z

src/models/StyledComponent.js

+          return
+        }
+
+        const { theme } = nextProps


Let's move this destructure to the function argument and use the theme variable throughout componentWillReceiveProps({ theme }).

Done. I hope flowtype is correct.

mxstbr · 2016-11-24T16:43:04Z

src/test/theme.test.js

+import jsdom from 'mocha-jsdom';
 import React from 'react'
-import { render } from 'enzyme'
+import { mount as render } from 'enzyme'


Why do we rename this to render? That's a separate mode, let's rename all instances of render to mount instead, otherwise this could be confusing.

Yeah I forgot to refactor this :)

mxstbr · 2016-11-24T17:12:36Z

src/models/StyledComponent.js

-          nextProps
-        )
-        this.setState({ generatedClassName })
+      componentWillReceiveProps({ theme }: { theme: ?{ [key: string]: mixed } }) {


I think this should be ({ theme }: { theme?: Object })? I'm not a flow type expert though, so maybe I'm wrong. @styled-components/typers how do we do this?

@mxstbr This annotation is the same as Theme in ThemeProvider. I can export Theme type from ThemeProvider component and use it here. Then it would be { theme: ?Theme }.

@mxstbr Theme type is now exported from ThemeProvider and used in StyledComponent and StyleNativeComponent. What do you think?

mxstbr · 2016-11-24T18:30:55Z

Oh sorry, I just realized that this will probably increase our rendering time since we're now doing a full jsdom render for every test. Do you mind adding a describe('jsdom', () => { jsdom() }) section in which we can put all the tests that require a full DOM render (i.e. mount), but use enyzmes render for all the original ones? That should speed it up quite a bit I think.

Sorry for the troubles, thanks!

michalkvasnicak · 2016-11-24T19:11:51Z

@mxstbr done 😉

…ps and re-render caused by props.theme update

…heme updated directly in props

…nt when theme is changed in props

michalkvasnicak · 2016-11-25T12:19:25Z

@mxstbr Do not merge this yet, there is bug in componentWillReceiveProps. We can't bail out at all because it will not react to props change so for example if you have a style that is using props to calculate something, then it won't propagate changes at all.

michalkvasnicak · 2016-11-25T12:31:36Z

@mxstbr ok fixed

mxstbr · 2016-11-26T09:41:17Z

Amazing work, thanks so much! Shipping a new version with these changes very soon!

mxstbr requested changes Nov 24, 2016

View reviewed changes

mxstbr mentioned this pull request Nov 24, 2016

Dynamicly update theme on react-storybook with knob #247

Closed

mxstbr reviewed Nov 24, 2016

View reviewed changes

michalkvasnicak added 10 commits November 24, 2016 20:29

Add failing tests for component re-render with theme from default pro…

95f3f22

…ps and re-render caused by props.theme update

Fix re-render with theme from default props and re-render caused by t…

dc59d18

…heme updated directly in props

Make StyledNativeComponent component behave the same as StyledCompone…

533df86

…nt when theme is changed in props

Add CHANGELOG.md entry

d35ce6c

Fix code style

1c3fb57

Bail out early if theme is not changed on update

3b858bd

Export Theme flow type definition to be reused in components

30d1d50

Fix flow type annotation

6519ace

Split theming tests to jsdom and non-jsdom suites

5c8f2a5

Rename test suite for jsdom theming tests

c02c708

Fix updating styles when props used in styles generation are updated

ef85058

mxstbr approved these changes Nov 26, 2016

View reviewed changes

mxstbr merged commit cd159c5 into styled-components:master Nov 26, 2016

This was referenced Nov 26, 2016

Theme is not updated properly #193

Closed

Theme stopped updating #261

Closed

michalkvasnicak deleted the fix/default-props-rerender branch November 27, 2016 10:24

Uh oh!

Conversation

michalkvasnicak commented Nov 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mxstbr-bot commented Nov 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mxstbr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mxstbr commented Nov 24, 2016

Uh oh!

michalkvasnicak commented Nov 24, 2016

Uh oh!

michalkvasnicak commented Nov 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michalkvasnicak commented Nov 25, 2016

Uh oh!

mxstbr commented Nov 26, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

michalkvasnicak commented Nov 24, 2016 •

edited

Loading

mxstbr-bot commented Nov 24, 2016 •

edited

Loading

michalkvasnicak commented Nov 25, 2016 •

edited

Loading