[WIP] (#1736) ability to force re-render a story #2463

dangreenisrael · 2017-12-11T23:00:18Z

Issue: #1736

What I did

added forceReRender to the exports in '@storybook/react'

How to test

Storyshots

Does this need a new example in the kitchen sink apps?

Yes

Does this need an update to the documentation?

Yes

Acceptance criteria:

Platforms

React
React Native
Angular
Vue

Functionality

Ability to re-render from in stories
Ability to re-render from addons via channel

Misc

Kitchen sink example (cra)
Screenshots

Sceenshot

Future possibility

Standard event that can be emitted to force a reload

Also adds support for redux DevTools in react/client

…tory

codecov · 2017-12-11T23:31:32Z

Codecov Report

Merging #2463 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2463      +/-   ##
==========================================
- Coverage   32.64%   32.61%   -0.03%     
==========================================
  Files         397      397              
  Lines        8847     8855       +8     
  Branches      951      959       +8     
==========================================
  Hits         2888     2888              
+ Misses       5292     5289       -3     
- Partials      667      678      +11

Impacted Files	Coverage Δ
app/react/src/client/preview/index.js	`0% <0%> (ø)`	⬆️
app/angular/src/client/preview/index.js	`0% <0%> (ø)`	⬆️
app/vue/src/client/preview/index.js	`0% <0%> (ø)`	⬆️
app/react/src/client/preview/render.js	`0% <0%> (ø)`	⬆️
app/vue/src/client/preview/render.js	`0% <0%> (ø)`	⬆️
app/react/src/client/index.js	`0% <0%> (ø)`	⬆️
app/angular/src/client/preview/render.js	`0% <0%> (ø)`	⬆️
app/react/src/server/config/babel.js	`0% <0%> (-100%)`	⬇️
app/react/src/server/utils.js	`0% <0%> (-53.58%)`	⬇️
lib/ui/src/modules/ui/containers/stories_panel.js	`25.71% <0%> (ø)`	⬆️
... and 48 more

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 47dc12e...82699d3. Read the comment docs.

danielduan · 2017-12-12T17:12:41Z

Think we should port this over for vue and angular as well if this was to be added as a feature.

dangreenisrael · 2017-12-12T18:12:05Z

@danielduan Will do. I'll port it to Vue and Angular when I get a chance, then I'll update the docs for all of them

ndelangen · 2017-12-14T22:19:12Z

app/react/src/client/preview/index.js

@@ -53,3 +59,5 @@ const renderUI = () => {
 };

 reduxStore.subscribe(renderUI);
+
+export const forceReRender = () => renderUI();


If the story didn't change, why would this work?

This code:
https://github.com/storybooks/storybook/blob/45f3a7f98e48c1bfeec0d7a2d9210e9701692cc3/app/react/src/client/preview/render.js#L58

Should return true, and the story won't be re-rendered?

Never mind, I mis-interpreted the code.

I was just looking at that code, funnily enough and I think it's a bit broken, I was going to send a PR to fix it.

The current (psuedo) logic is:

render() { // line linked above if (storyDifferent) { ReactDOM.unmountComponentAtNode(rootEl); } // later ReactDOM.render(element, rootEl); }

So it always calls render and sometimes calls unmount. This actually breaks the React lifecycle in weird ways.

So, if I was to fix the above (probably changing it to: if (!storyDifferent) return;, this PR would no longer work.

We should consider if we want to support this forceRerender functionality? I suppose we do? If so we will need a more declarative way to make it work, I don't think we should retain the above bug just to make it work.

Probably the easiest way would be to add a "key" to the redux store that is changed when you call forceRerender and to also check if the key has changed.

…tory

tmeasday · 2017-12-15T06:14:26Z

app/react/src/client/preview/index.js

+  window.__REDUX_DEVTOOLS_EXTENSION__ &&
+    window.__REDUX_DEVTOOLS_EXTENSION__({ name: 'Storybook Preview', instanceId: 'sbPreview' })
+);
+/* eslint-enable */


What's happening with this change? It seems unrelated.

Correct, it is unrelated. It's enabling dev tools. Do you think it should be in a separate branch/pr.

I think yeah, let's move to a separate PR

tmeasday · 2017-12-15T06:16:30Z

app/react/src/client/preview/index.js

@@ -53,3 +59,5 @@ const renderUI = () => {
 };

 reduxStore.subscribe(renderUI);
+
+export const forceReRender = () => renderUI();


I was just looking at that code, funnily enough and I think it's a bit broken, I was going to send a PR to fix it.

The current (psuedo) logic is:

render() { // line linked above if (storyDifferent) { ReactDOM.unmountComponentAtNode(rootEl); } // later ReactDOM.render(element, rootEl); }

So it always calls render and sometimes calls unmount. This actually breaks the React lifecycle in weird ways.

tmeasday · 2017-12-15T06:18:40Z

app/react/src/client/preview/index.js

@@ -53,3 +59,5 @@ const renderUI = () => {
 };

 reduxStore.subscribe(renderUI);
+
+export const forceReRender = () => renderUI();


So, if I was to fix the above (probably changing it to: if (!storyDifferent) return;, this PR would no longer work.

We should consider if we want to support this forceRerender functionality? I suppose we do? If so we will need a more declarative way to make it work, I don't think we should retain the above bug just to make it work.

tmeasday · 2017-12-15T06:21:39Z

@dangreenisrael I am making some of these changes on another branch (because I need the lifecycle to work properly) so LMK if it makes sense to port them across here.

tmeasday · 2017-12-15T06:23:51Z

PS as an aside -- is it expected that all new functionality needs to be implemented in all 3 (should it be 4, what about RN?) view layers to be shipped? For instance, I personally wouldn't have much idea how to (at least idiomatically) implement this in Vue and Angular. It seems like quite a barrier (maybe that's a good thing).

dangreenisrael · 2017-12-15T13:58:43Z

@tmeasday I'm planning to do a deep dive into force re-render over the winter holidays (probably December 27,28,29. Do you think your PR will be merged by then.

dangreenisrael · 2017-12-15T13:59:36Z

@tmeasday @ndelangen Do you guys want to set up a time to chat about force re-render so we can all get on the same page

tmeasday · 2017-12-18T03:57:38Z

@dangreenisrael - #2503

I'm pretty convinced we should merge the above. However, I think it shouldn't be too hard to amend the above to consider a renderKey or something from the redux store.

dangreenisrael · 2017-12-18T13:52:00Z

@tmeasday, I'll rebase off 3.3/your branch before getting back to work on this

…tory

ndelangen · 2017-12-21T10:00:01Z

Please check if this still works after @tmeasday's change..

tmeasday · 2017-12-27T03:09:22Z

Changes look good to me.

tmeasday

LGTM, let's get rid of the devtools

tmeasday · 2017-12-27T03:09:49Z

app/react/src/client/preview/index.js

+  window.__REDUX_DEVTOOLS_EXTENSION__ &&
+    window.__REDUX_DEVTOOLS_EXTENSION__({ name: 'Storybook Preview', instanceId: 'sbPreview' })
+);
+/* eslint-enable */


I think yeah, let's move to a separate PR

…#1736)-Ability-to-force-redender-a-story

ndelangen · 2017-12-28T14:55:05Z

…#1736)-Ability-to-force-redender-a-story

…#1736)-Ability-to-force-redender-a-story # Conflicts: # app/react/src/client/preview/render.js

I addressed the concerns, but his is unable to look at it right now.

dangreenisrael · 2017-12-28T18:40:18Z

Merging as @tmeasday's requested changes were made

stefanKuijers · 2018-02-08T15:06:29Z

Thank you guys for the great work. As far as I can see this has been merged in and master. I'm using version 3.3.12 but cannot run the example code.

@dangreenisrael: Sadly when using the example code I run into the following error
When I call forceReRender I get the following error:

    at increment (0.345baf011dfa0388ca2e.hot-update.js:70)
    at HTMLUnknownElement.callCallback (preview.bundle.js:30287)
    at Object.invokeGuardedCallbackDev (preview.bundle.js:30326)
    at Object.invokeGuardedCallback (preview.bundle.js:30183)
    at Object.invokeGuardedCallbackAndCatchFirstError (preview.bundle.js:30197)
    at executeDispatch (preview.bundle.js:30581)
    at executeDispatchesInOrder (preview.bundle.js:30603)
    at executeDispatchesAndRelease (preview.bundle.js:30701)
    at executeDispatchesAndReleaseTopLevel (preview.bundle.js:30712)
    at Array.forEach (<anonymous>)

It is thrown two times: 0.345baf011dfa0388ca2e.hot-update.js:70 & @ preview.bundle.js:30355.

When I look into the cra-kitchensink folder I don't see anything special in the setup which differs from mine. Is there still a change this is related to setup? For example that I'm using different loaders? (TS, SCSS)

Thanks in advance!

dangreenisrael · 2018-02-08T18:28:54Z

@stefanKuijers could you open up an issue for this? Also, can you reproduce this issue in a public repo?

Hypnosphi · 2018-02-08T18:34:29Z

@stefanKuijers This feature was merged after 3.3.0 release, so it targets 3.4.0 (currently in alpha, 3.4.0-alpha.7)

stefanKuijers · 2018-02-09T08:22:32Z

Thanks for the reply guys. I saw a few times in the branch names a 3.3 which is why I thought it was still part of 3.3.

Thanks for the great work! I will wait until 3.4.0. I see on the side now that this is assigned to the 3.4 milestone.

dangreenisrael and others added 2 commits December 11, 2017 17:58

(#1736) Adds Ability to re-render a story programatically in react

98fa5cd

Also adds support for redux DevTools in react/client

Merge branch 'release/3.3' into (#1736)-Ability-to-force-redender-a-s…

6cb8d21

…tory

(#1736) Add cra example for force re-render

45f3a7f

danielduan added feature request in progress labels Dec 12, 2017

ndelangen reviewed Dec 14, 2017

View reviewed changes

ndelangen assigned ndelangen and dangreenisrael Dec 14, 2017

Merge branch 'release/3.3' into (#1736)-Ability-to-force-redender-a-s…

50774cb

…tory

tmeasday requested changes Dec 15, 2017

View reviewed changes

tmeasday mentioned this pull request Dec 18, 2017

[SIGNIFICANT] Remove redundant render in react #2503

Merged

Merge branch 'release/3.3' into (#1736)-Ability-to-force-redender-a-s…

6d80f69

…tory

Hypnosphi changed the base branch from release/3.3 to master December 23, 2017 23:56

(#1736) Update forceReRender to not rely on a bug

d4609e4

tmeasday previously requested changes Dec 27, 2017

View reviewed changes

ndelangen and others added 3 commits December 27, 2017 14:47

Merge branch 'master' into (#1736)-Ability-to-force-redender-a-story

ce935e6

(#1736) Add forceReRender support for Angular and Vue

597387c

Merge branch 'master' of https://github.com/storybooks/storybook into (…

1fb6e3a

…#1736)-Ability-to-force-redender-a-story

dangreenisrael added the ci: do not merge label Dec 27, 2017

dangreenisrael and others added 2 commits December 27, 2017 11:15

(#1736) Update Force ReRender storyshot

31a68d7

Merge branch 'master' into (#1736)-Ability-to-force-redender-a-story

8e1caac

Hypnosphi added this to the v3.4.0 milestone Dec 27, 2017

dangreenisrael added ci: do not merge and removed ci: do not merge labels Dec 28, 2017

dangreenisrael added 2 commits December 28, 2017 10:27

Merge branch 'master' of https://github.com/storybooks/storybook into (…

293bb7a

…#1736)-Ability-to-force-redender-a-story

Merge branch 'master' of https://github.com/storybooks/storybook into (…

2be6ff2

…#1736)-Ability-to-force-redender-a-story # Conflicts: # app/react/src/client/preview/render.js

dangreenisrael removed ci: do not merge in progress labels Dec 28, 2017

dangreenisrael added 2 commits December 28, 2017 12:06

Merge branch 'master' into (#1736)-Ability-to-force-redender-a-story

a27b4bd

Merge branch 'master' into (#1736)-Ability-to-force-redender-a-story

82699d3

dangreenisrael merged commit 4b5622a into master Dec 28, 2017

dangreenisrael deleted the (#1736)-Ability-to-force-redender-a-story branch December 28, 2017 18:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] (#1736) ability to force re-render a story #2463

[WIP] (#1736) ability to force re-render a story #2463

dangreenisrael commented Dec 11, 2017 •

edited

Loading

codecov bot commented Dec 11, 2017 •

edited

Loading

danielduan commented Dec 12, 2017

dangreenisrael commented Dec 12, 2017 •

edited

Loading

ndelangen Dec 14, 2017 •

edited

Loading

ndelangen Dec 14, 2017

tmeasday Dec 15, 2017

tmeasday Dec 15, 2017

tmeasday Dec 15, 2017

tmeasday Dec 15, 2017

dangreenisrael Dec 15, 2017

tmeasday Dec 27, 2017

tmeasday Dec 15, 2017

tmeasday Dec 15, 2017

tmeasday commented Dec 15, 2017

tmeasday commented Dec 15, 2017

dangreenisrael commented Dec 15, 2017

dangreenisrael commented Dec 15, 2017

tmeasday commented Dec 18, 2017

dangreenisrael commented Dec 18, 2017

ndelangen commented Dec 21, 2017

tmeasday commented Dec 27, 2017

tmeasday left a comment

tmeasday Dec 27, 2017

ndelangen commented Dec 28, 2017

dangreenisrael commented Dec 28, 2017

stefanKuijers commented Feb 8, 2018

dangreenisrael commented Feb 8, 2018

Hypnosphi commented Feb 8, 2018 •

edited

Loading

stefanKuijers commented Feb 9, 2018

[WIP] (#1736) ability to force re-render a story #2463

[WIP] (#1736) ability to force re-render a story #2463

Conversation

dangreenisrael commented Dec 11, 2017 • edited Loading

What I did

How to test

Acceptance criteria:

Platforms

Functionality

Misc

Sceenshot

Future possibility

codecov bot commented Dec 11, 2017 • edited Loading

Codecov Report

danielduan commented Dec 12, 2017

dangreenisrael commented Dec 12, 2017 • edited Loading

ndelangen Dec 14, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmeasday commented Dec 15, 2017

tmeasday commented Dec 15, 2017

dangreenisrael commented Dec 15, 2017

dangreenisrael commented Dec 15, 2017

tmeasday commented Dec 18, 2017

dangreenisrael commented Dec 18, 2017

ndelangen commented Dec 21, 2017

tmeasday commented Dec 27, 2017

tmeasday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndelangen commented Dec 28, 2017

dangreenisrael commented Dec 28, 2017

stefanKuijers commented Feb 8, 2018

dangreenisrael commented Feb 8, 2018

Hypnosphi commented Feb 8, 2018 • edited Loading

stefanKuijers commented Feb 9, 2018

dangreenisrael commented Dec 11, 2017 •

edited

Loading

codecov bot commented Dec 11, 2017 •

edited

Loading

dangreenisrael commented Dec 12, 2017 •

edited

Loading

ndelangen Dec 14, 2017 •

edited

Loading

Hypnosphi commented Feb 8, 2018 •

edited

Loading