From e780cc5e672a1d94dd9bff7ee963492c8b0814db Mon Sep 17 00:00:00 2001 From: xavcz Date: Sun, 9 Jul 2017 17:23:25 -0400 Subject: [PATCH 1/4] fix(react-fiber): remove unnecessary element check in the preview rendering --- app/react/src/client/preview/render.js | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/app/react/src/client/preview/render.js b/app/react/src/client/preview/render.js index acd5c7a5a277..540533118485 100644 --- a/app/react/src/client/preview/render.js +++ b/app/react/src/client/preview/render.js @@ -83,17 +83,6 @@ export function renderMain(data, storyStore) { return renderError(error); } - if (element.type === undefined) { - const error = { - title: `Expecting a valid React element from the story: "${selectedStory}" of "${selectedKind}".`, - description: stripIndents` - Seems like you are not returning a correct React element from the story. - Could you double check that? - `, - }; - return renderError(error); - } - ReactDOM.render(element, rootEl); return null; } From ef61f91a16c73ed7e9d4b158a04de686e63bf8d1 Mon Sep 17 00:00:00 2001 From: xavcz Date: Mon, 10 Jul 2017 07:06:50 -0400 Subject: [PATCH 2/4] docs(storyshots): rAF polyfill if react 16 is used --- addons/storyshots/README.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/addons/storyshots/README.md b/addons/storyshots/README.md index a110dd6cceb3..813db6023f9f 100644 --- a/addons/storyshots/README.md +++ b/addons/storyshots/README.md @@ -32,6 +32,8 @@ Usually, you might already have completed this step. If not, here are some resou - If you are using Create React App, it's already configured for Jest. You just need to create a filename with the extension `.test.js`. - Otherwise check this Egghead [lesson](https://egghead.io/lessons/javascript-test-javascript-with-jest). +> Note: If you use React 16, you'll need to follow [these additional instructions](https://github.com/facebook/react/issues/9102#issuecomment-283873039). + ## Configure Storyshots Create a new test file with the name `Storyshots.test.js`. (Or whatever the name you prefer). From 67f496dc6d8211bda50fe778f1f9b29950ec972a Mon Sep 17 00:00:00 2001 From: xavcz Date: Sat, 15 Jul 2017 13:51:52 -0400 Subject: [PATCH 3/4] feat(app/react): check if the element is renderable / react version --- .../__snapshots__/element_check.test.js.snap | 13 +++ app/react/src/client/preview/element_check.js | 49 ++++++++++ .../src/client/preview/element_check.test.js | 98 +++++++++++++++++++ app/react/src/client/preview/render.js | 12 +++ 4 files changed, 172 insertions(+) create mode 100644 app/react/src/client/preview/__snapshots__/element_check.test.js.snap create mode 100644 app/react/src/client/preview/element_check.js create mode 100644 app/react/src/client/preview/element_check.test.js diff --git a/app/react/src/client/preview/__snapshots__/element_check.test.js.snap b/app/react/src/client/preview/__snapshots__/element_check.test.js.snap new file mode 100644 index 000000000000..ccc63779a37e --- /dev/null +++ b/app/react/src/client/preview/__snapshots__/element_check.test.js.snap @@ -0,0 +1,13 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`element_check.utils.flattenList should flatten a deep nested list 1`] = ` +Array [ + 1, + 2, + 3, + 4, + 5, + 6, + 7, +] +`; diff --git a/app/react/src/client/preview/element_check.js b/app/react/src/client/preview/element_check.js new file mode 100644 index 000000000000..be08335b26c0 --- /dev/null +++ b/app/react/src/client/preview/element_check.js @@ -0,0 +1,49 @@ +import React from 'react'; + +// return true if the element is renderable with react fiber +export const isValidFiberElement = element => + typeof element === 'string' || typeof element === 'number' || React.isValidElement(element); + +// input: [1, 2, [3, 4, [5, 6, [7]]]] +// output: [ 1, 2, 3, 4, 5, 6, 7 ] +export const flattenList = list => + list.reduce((array, element) => { + const newElement = Array.isArray(element) ? flattenList(element) : [element]; + + return [...array, ...newElement]; + }, []); + +export const isPriorToFiber = version => { + const [majorVersion] = version.split('.'); + + return Number(majorVersion) < 16; +}; + +// accepts an element and 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 (isPriorToFiber(React.version)) { + return React.isValidElement(element); + } + + // the element is not an array, check if its a fiber renderable element + if (!Array.isArray(element)) { + return isValidFiberElement(element); + } + + // the element is in fact a list of elements (array), + // loop on its elements to see if its ok to render them + const elementsList = element.map(isReactRenderable); + + // flatten the list of elements (possibly deep nested) + const flatList = flattenList(elementsList); + + // keep only invalid elements + const invalidElements = flatList.filter(elementIsRenderable => elementIsRenderable === false); + + // it's ok to render this list if there is no invalid elements inside + return !invalidElements.length; +}; + +export default isReactRenderable; diff --git a/app/react/src/client/preview/element_check.test.js b/app/react/src/client/preview/element_check.test.js new file mode 100644 index 000000000000..0b3695330054 --- /dev/null +++ b/app/react/src/client/preview/element_check.test.js @@ -0,0 +1,98 @@ +import React from 'react'; +import isReactRenderable, { + isValidFiberElement, + flattenList, + isPriorToFiber, +} from './element_check'; + +describe('element_check.utils.isValidFiberElement', () => { + it('should accept to render a string', () => { + const string = 'react is awesome'; + + expect(isValidFiberElement(string)).toBe(true); + }); + + it('should accept to render a number', () => { + const number = 42; + + expect(isValidFiberElement(number)).toBe(true); + }); + + it('should accept to render a valid React element', () => { + const element = ; + + expect(isValidFiberElement(element)).toBe(true); + }); + + it("shouldn't accept to render an arbitrary object", () => { + const object = { key: 'bee bop' }; + + expect(isValidFiberElement(object)).toBe(false); + }); + + it("shouldn't accept to render a function", () => { + const noop = () => {}; + + expect(isValidFiberElement(noop)).toBe(false); + }); + + it("shouldn't accept to render undefined", () => { + expect(isValidFiberElement(undefined)).toBe(false); + }); +}); + +describe('element_check.utils.flattenList', () => { + it('should flatten a deep nested list', () => { + const deepNestedList = [1, 2, [3, 4, [5, 6, [7]]]]; + + expect(flattenList(deepNestedList)).toMatchSnapshot(); + }); +}); + +describe('element_check.utils.isPriorToFiber', () => { + it('should return true if React version is prior to Fiber (< 16)', () => { + const oldVersion = '0.14.5'; + const version = '15.5.4'; + + expect(isPriorToFiber(oldVersion)).toBe(true); + expect(isPriorToFiber(version)).toBe(true); + }); + + it('should return false if React version is using Fiber features (>= 16)', () => { + const alphaVersion = '16.0.0-alpha.13'; + const version = '18.3.1'; + + expect(isPriorToFiber(alphaVersion)).toBe(false); + expect(isPriorToFiber(version)).toBe(false); + }); +}); + +describe('element_check.isReactRenderable', () => { + const string = 'yo'; + const number = 1337; + const element = what's up; + const array = [string, number, element]; + const object = { key: null }; + + it('allows rendering React elements only prior to React Fiber', () => { + // mutate version for the purpose of the test + React.version = '15.5.4'; + + expect(isReactRenderable(string)).toBe(false); + expect(isReactRenderable(number)).toBe(false); + expect(isReactRenderable(element)).toBe(true); + expect(isReactRenderable(array)).toBe(false); + expect(isReactRenderable(object)).toBe(false); + }); + + it('allows rendering string, numbers, arrays and React elements with React Fiber', () => { + // mutate version for the purpose of the test + React.version = '16.0.0-alpha.13'; + + expect(isReactRenderable(string)).toBe(true); + expect(isReactRenderable(number)).toBe(true); + expect(isReactRenderable(element)).toBe(true); + expect(isReactRenderable(array)).toBe(true); + expect(isReactRenderable(object)).toBe(false); + }); +}); diff --git a/app/react/src/client/preview/render.js b/app/react/src/client/preview/render.js index 540533118485..0e3754d19424 100644 --- a/app/react/src/client/preview/render.js +++ b/app/react/src/client/preview/render.js @@ -3,6 +3,7 @@ import React from 'react'; import ReactDOM from 'react-dom'; import { stripIndents } from 'common-tags'; +import isReactRenderable from './element_check'; import ErrorDisplay from './error_display'; // check whether we're running on node/browser @@ -83,6 +84,17 @@ export function renderMain(data, storyStore) { return renderError(error); } + if (!isReactRenderable(element)) { + const error = { + title: `Expecting a valid React element from the story: "${selectedStory}" of "${selectedKind}".`, + description: stripIndents` + Seems like you are not returning a correct React element from the story. + Could you double check that? + `, + }; + return renderError(error); + } + ReactDOM.render(element, rootEl); return null; } From 90c6253a81ec08a0d34e7b283893838717dfb9dc Mon Sep 17 00:00:00 2001 From: xavcz Date: Sat, 15 Jul 2017 15:21:11 -0400 Subject: [PATCH 4/4] fix(deps): don't reinvent the wh... uh, lodash.flattendeep! --- app/react/package.json | 1 + .../__snapshots__/element_check.test.js.snap | 13 ------------- app/react/src/client/preview/element_check.js | 12 ++---------- app/react/src/client/preview/element_check.test.js | 14 +------------- 4 files changed, 4 insertions(+), 36 deletions(-) delete mode 100644 app/react/src/client/preview/__snapshots__/element_check.test.js.snap diff --git a/app/react/package.json b/app/react/package.json index 25b0123e0347..094593541b24 100644 --- a/app/react/package.json +++ b/app/react/package.json @@ -53,6 +53,7 @@ "json-loader": "^0.5.4", "json-stringify-safe": "^5.0.1", "json5": "^0.5.1", + "lodash.flattendeep": "^4.4.0", "lodash.pick": "^4.4.0", "postcss-flexbugs-fixes": "^3.0.0", "postcss-loader": "^2.0.5", diff --git a/app/react/src/client/preview/__snapshots__/element_check.test.js.snap b/app/react/src/client/preview/__snapshots__/element_check.test.js.snap deleted file mode 100644 index ccc63779a37e..000000000000 --- a/app/react/src/client/preview/__snapshots__/element_check.test.js.snap +++ /dev/null @@ -1,13 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`element_check.utils.flattenList should flatten a deep nested list 1`] = ` -Array [ - 1, - 2, - 3, - 4, - 5, - 6, - 7, -] -`; diff --git a/app/react/src/client/preview/element_check.js b/app/react/src/client/preview/element_check.js index be08335b26c0..cbcb2f518da6 100644 --- a/app/react/src/client/preview/element_check.js +++ b/app/react/src/client/preview/element_check.js @@ -1,18 +1,10 @@ import React from 'react'; +import flattenDeep from 'lodash.flattendeep'; // return true if the element is renderable with react fiber export const isValidFiberElement = element => typeof element === 'string' || typeof element === 'number' || React.isValidElement(element); -// input: [1, 2, [3, 4, [5, 6, [7]]]] -// output: [ 1, 2, 3, 4, 5, 6, 7 ] -export const flattenList = list => - list.reduce((array, element) => { - const newElement = Array.isArray(element) ? flattenList(element) : [element]; - - return [...array, ...newElement]; - }, []); - export const isPriorToFiber = version => { const [majorVersion] = version.split('.'); @@ -37,7 +29,7 @@ const isReactRenderable = element => { const elementsList = element.map(isReactRenderable); // flatten the list of elements (possibly deep nested) - const flatList = flattenList(elementsList); + const flatList = flattenDeep(elementsList); // keep only invalid elements const invalidElements = flatList.filter(elementIsRenderable => elementIsRenderable === false); diff --git a/app/react/src/client/preview/element_check.test.js b/app/react/src/client/preview/element_check.test.js index 0b3695330054..674de0886a5d 100644 --- a/app/react/src/client/preview/element_check.test.js +++ b/app/react/src/client/preview/element_check.test.js @@ -1,9 +1,5 @@ import React from 'react'; -import isReactRenderable, { - isValidFiberElement, - flattenList, - isPriorToFiber, -} from './element_check'; +import isReactRenderable, { isValidFiberElement, isPriorToFiber } from './element_check'; describe('element_check.utils.isValidFiberElement', () => { it('should accept to render a string', () => { @@ -41,14 +37,6 @@ describe('element_check.utils.isValidFiberElement', () => { }); }); -describe('element_check.utils.flattenList', () => { - it('should flatten a deep nested list', () => { - const deepNestedList = [1, 2, [3, 4, [5, 6, [7]]]]; - - expect(flattenList(deepNestedList)).toMatchSnapshot(); - }); -}); - describe('element_check.utils.isPriorToFiber', () => { it('should return true if React version is prior to Fiber (< 16)', () => { const oldVersion = '0.14.5';