React fiber support #1443

xavxyz · 2017-07-10T11:19:45Z

Issue #724

😮 Problem

Let's assume you use react@16.x.y(-alpha-z) and you write a story as similar as this one:

storiesOf('Fiber rendering', module)
  .add('with array', () => 
    ['Hello Button', '😀 😎 👍 💯'].map(label => 
      <Button onClick={action(`clicked(${label})`)} key={label}>{label}</Button>
    )
  )
  .add('with string', () => 'Yay!')
  .add('with number', () => 1337)
  .add('with invalid render (object)', () => ({ invalid: true }));

In the React preview

There is a specific check on the element to render in the preview which prevents using the new "rendering features" of React Fiber: array, string, number.

If you write a story as similar as the one shown above, the UI will throw errors as seen in this screencast.

In Storyshots (-> Jest -> Node)

requestAnimationFrame is used in React Fiber, and doesn't exist in Node env, and so Jest (storyshots). It appears that it's something that needs to be fixed at the whole app level, it's not only a Storyshots issue. See screenshot.

✍️ What I did

on the UI

Simply remove the check, and let ReactDOM throws an error if the element is invalid.

Here is a screencast of what's happening without this check:

Without the element.type === undefined check:

for Storyshots (/ Jest)

If somebody decides to use React Fiber and Jest, they needs to manually mock requestAnimationFrame (at the moment! 🤙).

We can do like explained in this comment: facebook/react#9102 (comment).

// __mocks__/react.js
global.window = global;
window.addEventListener = () => {};
window.requestAnimationFrame = () => {
  throw new Error('requestAnimationFrame is not supported in Node');
};

📟 How to test

Setup using the `examples/test-cra` app

git clone https://github.com/storybooks/storybook
cd storybook
git checkout 724-react-fiber-support
cd examples/test-cra
yarn add react@alpha react-dom@alpha
yarn add --dev react-test-renderer@alpha

Add the following story to stories/index.js:

storiesOf('Fiber rendering', module)
  .add('with array', () => 
    ['Hello Button', '😀 😎 👍 💯'].map(label => 
      <Button onClick={action(`clicked(${label})`)} key={label}>{label}</Button>
    )
  )
  .add('with string', () => 'Yay!')
  .add('with number', () => 1337)
  .add('with invalid render (object)', () => ({ invalid: true }));

Preview test

yarn storybook

Then, go to http://localhost:9009.

Storyshots test

Create a __mocks_ folder in the root folder and add a react.js file in it:

mkdir __mocks__
touch react.js

Add the following in react.js:

global.window = global;
window.addEventListener = () => {};
window.requestAnimationFrame = () => {
  throw new Error('requestAnimationFrame is not supported in Node');
};

yarn test

❓ Questions

Should we keep the preview check and replace it by a function like isValidElement checking whether it's an element, a number, a string, or an array (and at elements inside it)?

What do you think? 🤔

…dering

codecov · 2017-07-10T11:19:48Z

Codecov Report

Merging #1443 into master will increase coverage by 0.19%.
The diff coverage is 51.85%.

@@            Coverage Diff             @@
##           master    #1443      +/-   ##
==========================================
+ Coverage   14.34%   14.54%   +0.19%     
==========================================
  Files         201      202       +1     
  Lines        4614     4649      +35     
  Branches      503      570      +67     
==========================================
+ Hits          662      676      +14     
+ Misses       3527     3474      -53     
- Partials      425      499      +74

Impacted Files	Coverage Δ
app/react/src/client/preview/render.js	`0% <0%> (ø)`	⬆️
app/react/src/client/preview/element_check.js	`41.17% <53.84%> (ø)`
app/react/src/server/iframe.html.js	`29.26% <0%> (ø)`	⬆️
.../ui/src/modules/ui/components/layout/dimensions.js	`15.62% <0%> (ø)`	⬆️
addons/info/src/components/PropTable.js	`0% <0%> (ø)`	⬆️
addons/knobs/src/components/types/Object.js	`5.81% <0%> (ø)`	⬆️
addons/knobs/src/KnobStore.js	`6.81% <0%> (ø)`	⬆️
addons/info/src/components/Node.js	`0% <0%> (ø)`	⬆️
app/react/src/client/preview/config_api.js	`0% <0%> (ø)`	⬆️
addons/info/src/components/markdown/htags.js	`0% <0%> (ø)`	⬆️
... and 25 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 c76bfb0...90c6253. Read the comment docs.

ndelangen · 2017-07-10T12:52:48Z

We should look for a react-provided isValidElement and use that, instead of writing our own.

shilman · 2017-07-10T13:54:32Z

@xavcz thanks so much for getting this going!

We obviously need to support new versions of React in storybook/storyshots. As far as stories themselves, might it make sense to restrict them to valid elements? String/number stories don't seem super useful, and array stories also seem dubious... So even though react supports them, does it mean we need to allow people to write stories with those?

ndelangen · 2017-07-10T14:22:06Z

does it mean we need to allow people to write stories with those ?

Yes, I believe so

shilman · 2017-07-10T17:28:48Z

Can we use this: https://facebook.github.io/react/docs/react-api.html#isvalidelement

xavxyz · 2017-07-10T18:23:06Z

React.isValidElement doesn't work for these new rendering features: it returns false.

Found some info there on the ReactDOMFiber & "(disable)newFiberFeatures": https://github.com/facebook/react/blob/master/src/renderers/dom/fiber/ReactDOMFiberEntry.js#L603-L634

shilman · 2017-07-10T19:23:14Z

@xavcz @ndelangen I still don't understand why it's necessary to support non-elements for the story renderer function just because it's possible in React fiber. If I write a component that's a react element that uses some of these new features, that will work without modifying storybook, correct?

tmeasday · 2017-07-11T01:32:03Z

@xavcz do you think they'll update isValidElement (or something equivalent, isReactRenderable or something) before they ship 16.0? I think we should probably handle whatever React can handle, but it would be nice to have some kind of check.

xavxyz · 2017-07-11T21:02:52Z

If I write a component that's a react element that uses some of these new features, that will work without modifying storybook, correct?

@shilman actually no! 😞 the preview's render checks on element.type (the one removed): a string, a number or an array don't have any type, only real React elements have a type! My assumption is that this check was introduced to prevent ugly big red error messages with long stack 👍

do you think they'll update isValidElement (or something equivalent, isReactRenderable or something) before they ship 16.0?

@tmeasday no idea! 😅 having an isReactRenderable function seems a good idea!

This would be something like:

// return true if the element is renderable with react fiber
const checkFiberRenderableTypes = element =>
  typeof element === 'string' ||
  typeof element === 'number' ||
  React.isValidElement(element);

// return true if renderable
// else return false
const isReactRenderable = element => {
  // storybook is running with a version prior to fiber, 
  // run a simple check on the element
  if (/* react version is below fiber */) {
    return React.isValidElement(element);
  }
  
  // the element is not an array, check if its a fiber renderable element 
  if (!Array.isArray(element)) {
    return checkFiberRenderableTypes(element);
  }

  // the element is an array, loop on its elements to see if its ok to render them
  return element
    // run this function on every item
    .map(isReactRenderable)
    // try to find a false value in this possibly deep nested array
    // -> if there is a false value, that means it cannot render
};

xavxyz · 2017-07-15T18:00:00Z

🔂 the element check before rendering the preview is back:

// before
if (element.type === undefined) {
  renderError(/* ... */)
}

// now
if (isReactRenderable(element)) {
  renderError(/* ... */)
}

This isReactRenderable function will:

run a React.isValidElement if the React version (React.version 🌮 ) in use is prior to Fiber
run a isValidFiberElement if the element is not an array (string, number or React element)
run isReactRenderable recursively if the element is an array.

I've added some tests for the utilities I created and the isReactRenderable function.

I've add a lot of fun doing this and learnt a lot! 😊

Based on the previous discussion, if you believe this check / PR is superfluous: feel free to close it, no bother 👌

shilman · 2017-07-15T18:05:37Z

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

+
+// input: [1, 2, [3, 4, [5, 6, [7]]]]
+// output: [ 1, 2, 3, 4, 5, 6, 7 ]
+export const flattenList = list =>


import { flattenDeep } from 'lodash'?

👌 definitely! I was hesitant to import lodash as I saw very few use of it in the project! 👍 will replace this function by lodash.flattenDeep!

Updated! ✅

shilman · 2017-07-15T18:07:26Z

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

+  const elementsList = element.map(isReactRenderable);
+
+  // flatten the list of elements (possibly deep nested)
+  const flatList = flattenList(elementsList);


why is it possibly deep nested if you are running element.map in the previous line?

Hehe, I personally wouldn't design a component deep nested, but it could be possible:

const WeirdComponent = () => [ <div>Some list</div>, [ <Something />, <div>This component is technically valid</div>, <em>But weird</em> ], <strong>Y U LIST</strong>, ];

Note: in this case, we should maybe throw an error: "Break this component in smaller ones for your sanity sake!" 😂

But if element is an array, then array.map(fn) is going to return an array, not a deeply-nested array?

oh sorry, it's recursive. nevermind 😄

shilman · 2017-07-15T18:09:02Z

@xavcz minor comments on your latest commit, otherwise I think it looks great!

shilman · 2017-07-15T19:24:37Z

Thanks @xavcz !!! Will merge as soon as the build finishes 👍 🎉 ❤️

xavxyz added 2 commits July 9, 2017 17:23

fix(react-fiber): remove unnecessary element check in the preview ren…

e780cc5

…dering

docs(storyshots): rAF polyfill if react 16 is used

ef61f91

xavxyz added react needs review labels Jul 10, 2017

arunoda added the in progress label Jul 10, 2017

xavxyz requested review from arunoda, shilman and ndelangen July 10, 2017 11:20

ndelangen approved these changes Jul 10, 2017

View reviewed changes

ndelangen requested review from tmeasday and removed request for arunoda July 10, 2017 12:53

ndelangen assigned xavxyz Jul 10, 2017

xavxyz added the feature request label Jul 10, 2017

Merge branch 'master' into 724-react-fiber-support

0722584

xavxyz added 2 commits July 15, 2017 13:47

Merge branch 'master' into 724-react-fiber-support

5b14eae

feat(app/react): check if the element is renderable / react version

67f496d

shilman reviewed Jul 15, 2017

View reviewed changes

fix(deps): don't reinvent the wh... uh, lodash.flattendeep!

90c6253

shilman merged commit e5a271b into master Jul 15, 2017

Hypnosphi deleted the 724-react-fiber-support branch August 17, 2017 22:28

This was referenced Sep 19, 2018

Update graphql in addons/graphql/package.json from ^0.13.2 to 14.0.2 Hypnosphi/storybook#5

Open

Update graphql in examples/official-storybook/package.json from ^0.13.2 to 14.0.2 Hypnosphi/storybook#6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

React fiber support #1443

React fiber support #1443

xavxyz commented Jul 10, 2017

codecov bot commented Jul 10, 2017 •

edited

ndelangen commented Jul 10, 2017

shilman commented Jul 10, 2017 •

edited

ndelangen commented Jul 10, 2017

shilman commented Jul 10, 2017

xavxyz commented Jul 10, 2017 •

edited

shilman commented Jul 10, 2017

tmeasday commented Jul 11, 2017

xavxyz commented Jul 11, 2017

xavxyz commented Jul 15, 2017

shilman Jul 15, 2017

xavxyz Jul 15, 2017

xavxyz Jul 15, 2017

shilman Jul 15, 2017 •

edited

xavxyz Jul 15, 2017

shilman Jul 15, 2017

shilman Jul 15, 2017

shilman commented Jul 15, 2017

shilman commented Jul 15, 2017

React fiber support #1443

React fiber support #1443

Conversation

xavxyz commented Jul 10, 2017

😮 Problem

In the React preview

In Storyshots (-> Jest -> Node)

✍️ What I did

on the UI

for Storyshots (/ Jest)

📟 How to test

Setup using the examples/test-cra app

Preview test

Storyshots test

❓ Questions

codecov bot commented Jul 10, 2017 • edited

Codecov Report

ndelangen commented Jul 10, 2017

shilman commented Jul 10, 2017 • edited

ndelangen commented Jul 10, 2017

shilman commented Jul 10, 2017

xavxyz commented Jul 10, 2017 • edited

shilman commented Jul 10, 2017

tmeasday commented Jul 11, 2017

xavxyz commented Jul 11, 2017

xavxyz commented Jul 15, 2017

shilman Jul 15, 2017

Choose a reason for hiding this comment

xavxyz Jul 15, 2017

Choose a reason for hiding this comment

xavxyz Jul 15, 2017

Choose a reason for hiding this comment

shilman Jul 15, 2017 • edited

Choose a reason for hiding this comment

xavxyz Jul 15, 2017

Choose a reason for hiding this comment

shilman Jul 15, 2017

Choose a reason for hiding this comment

shilman Jul 15, 2017

Choose a reason for hiding this comment

shilman commented Jul 15, 2017

shilman commented Jul 15, 2017

Setup using the `examples/test-cra` app

codecov bot commented Jul 10, 2017 •

edited

shilman commented Jul 10, 2017 •

edited

xavxyz commented Jul 10, 2017 •

edited

shilman Jul 15, 2017 •

edited