Fix knobs for React < 16.3 #3866

IanVS · 2018-07-10T10:39:23Z

Issue:

The getDerivedStateFromProps lifecycle method was added in React 16.3,
and older versions of react won’t execute it. Many of the knob components
were relying on this function to initialize component state, and so in
older versions of react state was undefined, and grabbing value would
blow up.

What I did

Most of the usages in these components were to memoize props, but that
can be handled more cleanly through the use of PureComponent, as
mentioned in:
https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#what-about-memoization

I wasn't sure quite how to avoid derived state in the object knob, so instead I followed the same approach as #3829 and polyfilled it.

How to test

The only thing I can think of is to test with a version of react prior to the introduction of getDerivedStateFromProps. I'm unfamiliar with your testing setup though, so I don't know how to accomplish this in an automated method. Do you already have any smoke tests for older react versions? If so, perhaps it's just a matter of a simple UI test of these components to ensure they don't go up in flames?

Is this testable with Jest or Chromatic screenshots? I don't know. :(
Does this need a new example in the kitchen sink apps? No
Does this need an update to the documentation? No

The `getDerivedStateFromProps` lifecycle method was added in React 16.3, and older versions of react won’t execute it. Many of the knob components were relying on this function to initialize component state, and so in older versions of react state was undefined, and grabbing `value` would blow up. Most of the usages in these components were to memoize props, but that can be handled more cleanly through the use of `PureComponent`, as mentioned in: https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#what-about-memoization

IanVS · 2018-07-10T10:52:01Z

I also pushed a commit in an attempt to fix the error I got from CircleCI about a dirty repo. I don't see how this could be related to any of my changes, but maybe???

wuweiweiwu · 2018-07-12T08:14:30Z

addons/knobs/src/components/types/Array.js

@@ -10,28 +10,18 @@ function formatArray(value, separator) {
  return value.split(separator);
 }

-class ArrayType extends React.Component {


I'm not sure how useful PureComponent is here.

Since knobs is a object, shallow comparisons done by PureComponent are always going to return false. and maybe will even have a slight performance hit since we're always doing a shallow comparison of prop values

Ah, yeah I'm not so familiar with the inner workings of knobs. Are you saying that the knob prop is re-generated very often (each time this component re-renders)?

Perhaps a better solution would be a custom shouldComponentUpdate?

I mean that using PureComponent is unnecessary since the shallow comparison of props (in this case this.props.knob) is always going to return false

Thus they should remain Component

There is no need for an implementation of shouldComponentUpdate

wuweiweiwu · 2018-07-12T09:04:39Z

After minor comment is addressed LGTM

It seems that some were not being accounted for.

Because the `knob` object prop is being regenerated each time the parent re-renders, we can’t use the shallow prop comparison that PureComponent does. So this uses a `shouldComponentUpdate` instead, which in most cases just compares the `knob.value` prop to see if it has changed. I don’t believe that the `name` is changed between renders. Same goes for number range configurations and array separators.

codecov · 2018-07-12T09:53:31Z

Codecov Report

Merging #3866 into master will increase coverage by 0.11%.
The diff coverage is 18.18%.

@@            Coverage Diff             @@
##           master    #3866      +/-   ##
==========================================
+ Coverage   41.44%   41.55%   +0.11%     
==========================================
  Files         455      455              
  Lines        5193     5176      -17     
  Branches      903      898       -5     
==========================================
- Hits         2152     2151       -1     
+ Misses       2496     2485      -11     
+ Partials      545      540       -5

Impacted Files	Coverage Δ
addons/knobs/src/components/types/Text.js	`40% <0%> (+11.42%)`	⬆️
addons/knobs/src/components/types/Date/index.js	`44.44% <0%> (+11.11%)`	⬆️
addons/knobs/src/components/types/Number.js	`46.66% <0%> (+9.82%)`	⬆️
addons/knobs/src/components/types/Color.js	`30% <0%> (+3.91%)`	⬆️
addons/knobs/src/components/types/Object.js	`22.22% <50%> (+4.57%)`	⬆️
addons/knobs/src/components/types/Array.js	`88.88% <50%> (+7.07%)`	⬆️

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 957c74e...1652d64. Read the comment docs.

IanVS · 2018-07-12T09:56:30Z

As @wuweiweiwu pointed out, it's not possible to rely on PureComponent in these components. It's because the parent is re-creating it for each render: https://github.com/storybooks/storybook/blob/cdca2e892c762ccd89b2fc08eaefc6407ff7c2c4/addons/knobs/src/components/Panel.js#L127.

I did a little experimenting, and sure enough the components were re-rendering when a different knob was changed. So I added a shouldComponentUpdate to check only whether the knob.value was changed. I don't think that knob.name is generally changed between renders, right? Same goes for array's knob.separator and number's knob.range and friends. If that's a bad assumption, let me know and I can update the methods.

wuweiweiwu · 2018-07-13T01:02:21Z

@IanVS I think you shouldn't implement shouldComponentUpdate at all in knobs.

Since each component is being recreated every single time a value changed. shouldComponentUpdate would be redundant.

IanVS · 2018-07-13T06:17:58Z

When I tested it, the shouldComponentUpdate was indeed able to prevent unnecessary re-renders.

@ndelangen has done a lot of performance work on these components, perhaps he has some insight here?

I'm happy to take them out if that is the consensus, but I would hate for this PR to have the end effect of harming performance for all users (since it will be removing the memoizing effect of getDerivedStateFromProps).

wuweiweiwu · 2018-07-13T07:11:48Z

@IanVS I'm not sure what you mean by the memoizing effect of gDSFP. It doesn't have any performance impacts since it only calculates state based on prop on every time render is called. And the only way to make sure some props have not changed is storing it in state since this is not available in a static function. There was actually an RFC to include prevProps but it was abandoned.

Unless you're implementing sCU - which can reduce extra renders. However, in this case, we can't rely on assumptions that knob.name or knob.range won't commonly change because they can and we will need to re-render if they do.

~~You can see here that the only prop we would have to compare is knobs and since we care about all the values in this.props.knobs it doesn't make sense to implement sCU.~~

I hope my point is clear. I'm happy to answer any questions.

edit: to clarify. I think your current changes is good ~~but an implementation of sCU is unnecessary~~ but if you wanted to implement sCU you would have to compare all the prop values that are used in the component.

ndelangen

Performance seems really good in the official example so LGTM @IanVS

IanVS · 2018-07-18T09:40:52Z

I'm a bit concerned that the changes here will prevent renders if the name of knobs is being dynamically changed (which seems like a weird thing to do, but maybe there's a use case?) I can submit another PR to address that requirement if desired.

Barring that, is there a chance to get a new alpha release? Our stories are currently broken until this can go out (I can't figure out how to point our app at my own fork, since it's a lerna project).

Hypnosphi · 2018-08-03T11:15:13Z

Released as 4.0.0-alpha.15

IanVS requested review from alexandrebodin, alterx, Hypnosphi, leonrodenburg and theinterned as code owners July 10, 2018 10:39

Fix dirty lockfile

4629403

wuweiweiwu added addon: knobs bug labels Jul 12, 2018

wuweiweiwu reviewed Jul 12, 2018

View reviewed changes

IanVS added 2 commits July 12, 2018 11:47

Expand NumberType PropTypes

554da3b

It seems that some were not being accounted for.

sanpoChew mentioned this pull request Jul 13, 2018

Fix addons panel when using preact #3882

Merged

ndelangen approved these changes Jul 14, 2018

View reviewed changes

ndelangen merged commit 42a7769 into storybookjs:master Jul 14, 2018

IanVS deleted the fix-knobs-for-older-react branch July 18, 2018 09:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix knobs for React < 16.3 #3866

Fix knobs for React < 16.3 #3866

IanVS commented Jul 10, 2018

IanVS commented Jul 10, 2018 •

edited

Loading

wuweiweiwu Jul 12, 2018

IanVS Jul 12, 2018 •

edited

Loading

wuweiweiwu Jul 12, 2018

wuweiweiwu commented Jul 12, 2018

codecov bot commented Jul 12, 2018 •

edited

Loading

IanVS commented Jul 12, 2018

wuweiweiwu commented Jul 13, 2018

IanVS commented Jul 13, 2018

wuweiweiwu commented Jul 13, 2018 •

edited

Loading

ndelangen left a comment

IanVS commented Jul 18, 2018

Hypnosphi commented Aug 3, 2018 •

edited

Loading

Fix knobs for React < 16.3 #3866

Fix knobs for React < 16.3 #3866

Conversation

IanVS commented Jul 10, 2018

What I did

How to test

IanVS commented Jul 10, 2018 • edited Loading

wuweiweiwu Jul 12, 2018

Choose a reason for hiding this comment

IanVS Jul 12, 2018 • edited Loading

Choose a reason for hiding this comment

wuweiweiwu Jul 12, 2018

Choose a reason for hiding this comment

wuweiweiwu commented Jul 12, 2018

codecov bot commented Jul 12, 2018 • edited Loading

Codecov Report

IanVS commented Jul 12, 2018

wuweiweiwu commented Jul 13, 2018

IanVS commented Jul 13, 2018

wuweiweiwu commented Jul 13, 2018 • edited Loading

ndelangen left a comment

Choose a reason for hiding this comment

IanVS commented Jul 18, 2018

Hypnosphi commented Aug 3, 2018 • edited Loading

IanVS commented Jul 10, 2018 •

edited

Loading

IanVS Jul 12, 2018 •

edited

Loading

codecov bot commented Jul 12, 2018 •

edited

Loading

wuweiweiwu commented Jul 13, 2018 •

edited

Loading

Hypnosphi commented Aug 3, 2018 •

edited

Loading