From 3089db94ceacc5792d811d24423349fbbbaf78a1 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 30 Jan 2017 11:29:14 -0800 Subject: [PATCH 1/6] Throw on invalid object type children Same behavior as Stack --- src/renderers/shared/fiber/ReactChildFiber.js | 46 ++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index 0505ae07e34c..e40a48c56403 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -36,9 +36,11 @@ var emptyObject = require('emptyObject'); var getIteratorFn = require('getIteratorFn'); var invariant = require('invariant'); var ReactFeatureFlags = require('ReactFeatureFlags'); +var ReactCurrentOwner = require('ReactCurrentOwner'); if (__DEV__) { var { getCurrentFiberStackAddendum } = require('ReactDebugCurrentFiber'); + var { getComponentName } = require('ReactFiberTreeReflection'); var warning = require('warning'); } @@ -107,6 +109,37 @@ function coerceRef(current: ?Fiber, element: ReactElement) { return mixedRef; } +function throwOnInvalidObjectType(newChild : Object) { + const childrenString = String(newChild); + let addendum = ''; + if (__DEV__) { + addendum = + ' If you meant to render a collection of children, use an array ' + + 'instead or wrap the object using createFragment(object) from the ' + + 'React add-ons.'; + if (newChild._isReactElement) { + addendum = + ' It looks like you\'re using an element created by a different ' + + 'version of React. Make sure to use only one copy of React.'; + } + if (ReactCurrentOwner.current) { + const owner : Fiber = (ReactCurrentOwner.current : any); + let name = getComponentName(owner); + if (name) { + addendum += ' Check the render method of `' + name + '`.'; + } + } + } + invariant( + false, + 'Objects are not valid as a React child (found: %s).%s', + childrenString === '[object Object]' ? + 'object with keys {' + Object.keys(newChild).join(', ') + '}' : + childrenString, + addendum + ); +} + // This wrapper function exists because I expect to clone the code in each path // to be able to optimize each path individually by branching early. This needs // a compiler or we can do it manually. Helpers that don't need this branching @@ -418,6 +451,8 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { created.return = returnFiber; return created; } + + throwOnInvalidObjectType(newChild); } return null; @@ -481,6 +516,8 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { } return updateFragment(returnFiber, oldFiber, newChild, priority); } + + throwOnInvalidObjectType(newChild); } return null; @@ -536,6 +573,8 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { const matchedFiber = existingChildren.get(newIdx) || null; return updateFragment(returnFiber, matchedFiber, newChild, priority); } + + throwOnInvalidObjectType(newChild); } return null; @@ -1083,7 +1122,8 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { const disableNewFiberFeatures = ReactFeatureFlags.disableNewFiberFeatures; // Handle object types - if (typeof newChild === 'object' && newChild !== null) { + const isObject = typeof newChild === 'object' && newChild !== null; + if (isObject) { // Support only the subset of return types that Stack supports. Treat // everything else as empty, but log a warning. if (disableNewFiberFeatures) { @@ -1199,6 +1239,10 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { ); } + if (isObject) { + throwOnInvalidObjectType(newChild); + } + if (!disableNewFiberFeatures && typeof newChild === 'undefined') { // If the new child is undefined, and the return fiber is a composite // component, throw an error. If Fiber return types are disabled, From 802c820545b978726388620c35ab4cc35370462b Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 30 Jan 2017 11:52:40 -0800 Subject: [PATCH 2/6] Fallback to owner of returnFiber Won't work if the returnFiber is a fragment. Not sure it matters; warning will just be less descriptive. --- src/renderers/shared/fiber/ReactChildFiber.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index e40a48c56403..9de22a88df8c 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -109,7 +109,7 @@ function coerceRef(current: ?Fiber, element: ReactElement) { return mixedRef; } -function throwOnInvalidObjectType(newChild : Object) { +function throwOnInvalidObjectType(returnFiber : Fiber, newChild : Object) { const childrenString = String(newChild); let addendum = ''; if (__DEV__) { @@ -122,9 +122,9 @@ function throwOnInvalidObjectType(newChild : Object) { ' It looks like you\'re using an element created by a different ' + 'version of React. Make sure to use only one copy of React.'; } - if (ReactCurrentOwner.current) { - const owner : Fiber = (ReactCurrentOwner.current : any); - let name = getComponentName(owner); + const owner = ReactCurrentOwner.owner || returnFiber._debugOwner; + if (owner && typeof owner.tag === 'number') { + const name = getComponentName((owner : any)); if (name) { addendum += ' Check the render method of `' + name + '`.'; } @@ -452,7 +452,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { return created; } - throwOnInvalidObjectType(newChild); + throwOnInvalidObjectType(returnFiber, newChild); } return null; @@ -517,7 +517,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { return updateFragment(returnFiber, oldFiber, newChild, priority); } - throwOnInvalidObjectType(newChild); + throwOnInvalidObjectType(returnFiber, newChild); } return null; @@ -574,7 +574,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { return updateFragment(returnFiber, matchedFiber, newChild, priority); } - throwOnInvalidObjectType(newChild); + throwOnInvalidObjectType(returnFiber, newChild); } return null; @@ -1240,7 +1240,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { } if (isObject) { - throwOnInvalidObjectType(newChild); + throwOnInvalidObjectType(returnFiber, newChild); } if (!disableNewFiberFeatures && typeof newChild === 'undefined') { From 7af673afc148fe2eb8a1753a901c1d46b78ea476 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 30 Jan 2017 12:21:14 -0800 Subject: [PATCH 3/6] Allow object as textarea child Preserves Stack behavior. There's already a warning for this. --- src/renderers/shared/fiber/ReactChildFiber.js | 50 ++++++++++--------- 1 file changed, 26 insertions(+), 24 deletions(-) diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index 9de22a88df8c..e96f04d02680 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -110,34 +110,36 @@ function coerceRef(current: ?Fiber, element: ReactElement) { } function throwOnInvalidObjectType(returnFiber : Fiber, newChild : Object) { - const childrenString = String(newChild); - let addendum = ''; - if (__DEV__) { - addendum = - ' If you meant to render a collection of children, use an array ' + - 'instead or wrap the object using createFragment(object) from the ' + - 'React add-ons.'; - if (newChild._isReactElement) { + if (returnFiber.type !== 'textarea') { + const childrenString = String(newChild); + let addendum = ''; + if (__DEV__) { addendum = - ' It looks like you\'re using an element created by a different ' + - 'version of React. Make sure to use only one copy of React.'; - } - const owner = ReactCurrentOwner.owner || returnFiber._debugOwner; - if (owner && typeof owner.tag === 'number') { - const name = getComponentName((owner : any)); - if (name) { - addendum += ' Check the render method of `' + name + '`.'; + ' If you meant to render a collection of children, use an array ' + + 'instead or wrap the object using createFragment(object) from the ' + + 'React add-ons.'; + if (newChild._isReactElement) { + addendum = + ' It looks like you\'re using an element created by a different ' + + 'version of React. Make sure to use only one copy of React.'; + } + const owner = ReactCurrentOwner.owner || returnFiber._debugOwner; + if (owner && typeof owner.tag === 'number') { + const name = getComponentName((owner : any)); + if (name) { + addendum += ' Check the render method of `' + name + '`.'; + } } } + invariant( + false, + 'Objects are not valid as a React child (found: %s).%s', + childrenString === '[object Object]' ? + 'object with keys {' + Object.keys(newChild).join(', ') + '}' : + childrenString, + addendum + ); } - invariant( - false, - 'Objects are not valid as a React child (found: %s).%s', - childrenString === '[object Object]' ? - 'object with keys {' + Object.keys(newChild).join(', ') + '}' : - childrenString, - addendum - ); } // This wrapper function exists because I expect to clone the code in each path From 7daa90206def22ce0201696349049347ac156a2d Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 30 Jan 2017 12:22:09 -0800 Subject: [PATCH 4/6] Add missing portal case to updateSlot switch statement --- src/renderers/shared/fiber/ReactChildFiber.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index e96f04d02680..ac8a4b2acbfd 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -508,6 +508,14 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { return null; } } + + case REACT_PORTAL_TYPE: { + if (newChild.key === key) { + return updatePortal(returnFiber, oldFiber, newChild, priority); + } else { + return null; + } + } } if (isArray(newChild) || getIteratorFn(newChild)) { From 17ec96b01459365f6b8164e0a2a4c7feb3ad6fc8 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 30 Jan 2017 12:22:25 -0800 Subject: [PATCH 5/6] Run test script --- scripts/fiber/tests-failing.txt | 5 ----- scripts/fiber/tests-passing.txt | 3 +++ 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 7ab7803ab368..d14ca51e78db 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -1,8 +1,3 @@ -src/addons/__tests__/ReactFragment-test.js -* should throw if a plain object is used as a child -* should throw if a plain object even if it is in an owner -* should throw if a plain object looks like an old element - src/isomorphic/classic/__tests__/ReactContextValidator-test.js * should pass previous context to lifecycles diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index a9d16d92ebc4..0c26cc1bca62 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -39,6 +39,9 @@ src/addons/__tests__/ReactComponentWithPureRenderMixin-test.js * does not do a deep comparison src/addons/__tests__/ReactFragment-test.js +* should throw if a plain object is used as a child +* should throw if a plain object even if it is in an owner +* should throw if a plain object looks like an old element * warns for numeric keys on objects as children * should warn if passing null to createFragment * should warn if passing an array to createFragment From 2a0fb5b4aaea06755268613a58eadb065a860d93 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 31 Jan 2017 11:46:30 -0800 Subject: [PATCH 6/6] Remove legacy React element warning --- scripts/fiber/tests-passing.txt | 1 - src/addons/__tests__/ReactFragment-test.js | 11 ----------- src/renderers/shared/fiber/ReactChildFiber.js | 5 ----- src/shared/utils/traverseAllChildren.js | 5 ----- 4 files changed, 22 deletions(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 0c26cc1bca62..04c72a219f93 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -41,7 +41,6 @@ src/addons/__tests__/ReactComponentWithPureRenderMixin-test.js src/addons/__tests__/ReactFragment-test.js * should throw if a plain object is used as a child * should throw if a plain object even if it is in an owner -* should throw if a plain object looks like an old element * warns for numeric keys on objects as children * should warn if passing null to createFragment * should warn if passing an array to createFragment diff --git a/src/addons/__tests__/ReactFragment-test.js b/src/addons/__tests__/ReactFragment-test.js index 4788c06e2f3d..0f86093b7ba6 100644 --- a/src/addons/__tests__/ReactFragment-test.js +++ b/src/addons/__tests__/ReactFragment-test.js @@ -59,17 +59,6 @@ describe('ReactFragment', () => { ); }); - it('should throw if a plain object looks like an old element', () => { - var oldEl = {_isReactElement: true, type: 'span', props: {}}; - var container = document.createElement('div'); - expect(() => ReactDOM.render(
{oldEl}
, container)).toThrowError( - 'Objects are not valid as a React child (found: object with keys ' + - '{_isReactElement, type, props}). It looks like you\'re using an ' + - 'element created by a different version of React. Make sure to use ' + - 'only one copy of React.' - ); - }); - it('warns for numeric keys on objects as children', () => { spyOn(console, 'error'); diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index ac8a4b2acbfd..db8be5c9f908 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -118,11 +118,6 @@ function throwOnInvalidObjectType(returnFiber : Fiber, newChild : Object) { ' If you meant to render a collection of children, use an array ' + 'instead or wrap the object using createFragment(object) from the ' + 'React add-ons.'; - if (newChild._isReactElement) { - addendum = - ' It looks like you\'re using an element created by a different ' + - 'version of React. Make sure to use only one copy of React.'; - } const owner = ReactCurrentOwner.owner || returnFiber._debugOwner; if (owner && typeof owner.tag === 'number') { const name = getComponentName((owner : any)); diff --git a/src/shared/utils/traverseAllChildren.js b/src/shared/utils/traverseAllChildren.js index 60b5f147aabf..9ea69721d07c 100644 --- a/src/shared/utils/traverseAllChildren.js +++ b/src/shared/utils/traverseAllChildren.js @@ -167,11 +167,6 @@ function traverseAllChildrenImpl( ' If you meant to render a collection of children, use an array ' + 'instead or wrap the object using createFragment(object) from the ' + 'React add-ons.'; - if (children._isReactElement) { - addendum = - ' It looks like you\'re using an element created by a different ' + - 'version of React. Make sure to use only one copy of React.'; - } if (ReactCurrentOwner.current) { var name = ReactCurrentOwner.current.getName(); if (name) {