Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 15 commits into from
Dec 28, 2017
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
98fa5cd
(#1736) Adds Ability to re-render a story programatically in react
dangreenisrael Dec 11, 2017
6cb8d21
Merge branch 'release/3.3' into (#1736)-Ability-to-force-redender-a-s…
dangreenisrael Dec 11, 2017
45f3a7f
(#1736) Add cra example for force re-render
dangreenisrael Dec 11, 2017
50774cb
Merge branch 'release/3.3' into (#1736)-Ability-to-force-redender-a-s…
ndelangen Dec 14, 2017
6d80f69
Merge branch 'release/3.3' into (#1736)-Ability-to-force-redender-a-s…
ndelangen Dec 21, 2017
d4609e4
(#1736) Update forceReRender to not rely on a bug
dangreenisrael Dec 27, 2017
ce935e6
Merge branch 'master' into (#1736)-Ability-to-force-redender-a-story
ndelangen Dec 27, 2017
597387c
(#1736) Add forceReRender support for Angular and Vue
dangreenisrael Dec 27, 2017
1fb6e3a
Merge branch 'master' of https://github.com/storybooks/storybook into…
dangreenisrael Dec 27, 2017
31a68d7
(#1736) Update Force ReRender storyshot
dangreenisrael Dec 27, 2017
8e1caac
Merge branch 'master' into (#1736)-Ability-to-force-redender-a-story
dangreenisrael Dec 27, 2017
293bb7a
Merge branch 'master' of https://github.com/storybooks/storybook into…
dangreenisrael Dec 28, 2017
2be6ff2
Merge branch 'master' of https://github.com/storybooks/storybook into…
dangreenisrael Dec 28, 2017
a27b4bd
Merge branch 'master' into (#1736)-Ability-to-force-redender-a-story
dangreenisrael Dec 28, 2017
82699d3
Merge branch 'master' into (#1736)-Ability-to-force-redender-a-story
dangreenisrael Dec 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion app/react/src/client/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,14 @@ import deprecate from 'util-deprecate';
import { action as deprecatedAction } from '@storybook/addon-actions';
import { linkTo as deprecatedLinkTo } from '@storybook/addon-links';

export { storiesOf, setAddon, addDecorator, configure, getStorybook } from './preview';
export {
storiesOf,
setAddon,
addDecorator,
configure,
getStorybook,
forceReRender,
} from './preview';

export const action = deprecate(
deprecatedAction,
Expand Down
10 changes: 9 additions & 1 deletion app/react/src/client/preview/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,13 @@ const isBrowser =
!(navigator.userAgent.indexOf('Node.js') > -1);

const storyStore = new StoryStore();
const reduxStore = createStore(reducer);
/* eslint-disable no-underscore-dangle */
const reduxStore = createStore(
reducer,
window.__REDUX_DEVTOOLS_EXTENSION__ &&
window.__REDUX_DEVTOOLS_EXTENSION__({ name: 'Storybook Preview', instanceId: 'sbPreview' })
);
/* eslint-enable */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const context = { storyStore, reduxStore };

if (isBrowser) {
Expand Down Expand Up @@ -53,3 +59,5 @@ const renderUI = () => {
};

reduxStore.subscribe(renderUI);

export const forceReRender = () => renderUI();
Copy link
Member

@ndelangen ndelangen Dec 14, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind, I mis-interpreted the code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

12 changes: 12 additions & 0 deletions examples/cra-kitchen-sink/src/stories/force-rerender.stories.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import React from 'react';
import { storiesOf, forceReRender } from '@storybook/react';

let count = 0;
const increment = () => {
count += 1;
forceReRender();
};

storiesOf('Force ReRender', module).add('button', () => (
<button onClick={increment}> Click me to increment: {count} </button>
));