[SIGNIFICANT] Remove redundant render in react #2503
Conversation
tmeasday
changed the title
| previousKind = selectedKind; | ||
| previousStory = selectedStory; | ||
| ReactDOM.unmountComponentAtNode(rootEl); | ||
| if (selectedKind === previousKind || previousStory === selectedStory) { |
| import React, { Component } from 'react'; | ||
|
|
||
| function log(name) { | ||
| console.log(`LifecycleLogger: ${name}`); |
There was a problem hiding this comment.
Do you think I should use addon-actions for this instead?
There was a problem hiding this comment.
You can just ignore the warning =)
There was a problem hiding this comment.
You should use @storybook/client-logger
Codecov Report
@@ Coverage Diff @@
## release/3.3 #2503 +/- ##
============================================
Coverage 19.45% 19.45%
============================================
Files 387 387
Lines 8734 8734
Branches 947 949 +2
============================================
Hits 1699 1699
+ Misses 6319 6307 -12
- Partials 716 728 +12
Continue to review full report at Codecov.
|
|
I wonder if it's easy to add some test that will really check the life cycle. |
Did you have an idea for this? It's tricky because it's testing the top-level of the |
|
Any other opinions on this @storybooks/team? Shall we merge it? |
|
Yeah this makes sense, we want this. Slightly concerned this will break some plugins, if they depend on this behavior. But let's merge and fix the addons if it happens. Related: #2463 |
ndelangen
changed the title
Issue:
In the React framework code,
ReactDOM.renderwould get unnecessarily called twice.What I did
Exit early from the render function if nothing is changing.
How to test
I added a story "Lifecycle:logging" to the cra-kitchen-sink app that demostrates the problem:
To reproduce: browse to the story, hit reload in the browser (as it happens whenever the preview redux store changes, there are a probably other ways to reproduce).
Previously: the component would re-render, which would log extra lines to the console.
Now: it simply renders once.
We should also satisfy ourselves that no other feature was relying on this bug (cf #2463)
Is this testable with jest or storyshots?
Not right now; it would be good to maybe use JSDOM to test this kind of thing.
Does this need a new example in the kitchen sink apps?
Yes, done
Does this need an update to the documentation?
No
If your answer is yes to any of these, please make sure to include it in your PR.