From f141f1888801cea1042b2e105a51024d724a4262 Mon Sep 17 00:00:00 2001 From: Toru Kobayashi Date: Mon, 28 Nov 2016 14:24:16 +0900 Subject: [PATCH] Fix Fiber tests for update callback warnings --- scripts/fiber/tests-failing.txt | 7 ----- scripts/fiber/tests-passing.txt | 5 ++++ src/renderers/dom/fiber/ReactDOMFiber.js | 26 +++++++++++++---- src/renderers/noop/ReactNoop.js | 14 ++++++--- .../shared/fiber/ReactFiberClassComponent.js | 4 +-- .../shared/fiber/ReactFiberReconciler.js | 20 ++++++++++--- .../shared/fiber/ReactFiberUpdateQueue.js | 29 ++++++++++++++++++- 7 files changed, 81 insertions(+), 24 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 195917ac4e016..2ad146ffafabb 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -24,10 +24,6 @@ src/renderers/art/__tests__/ReactART-test.js * resolves refs before componentDidMount * resolves refs before componentDidUpdate -src/renderers/dom/shared/__tests__/ReactDOM-test.js -* throws in render() if the mount callback is not a function -* throws in render() if the update callback is not a function - src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js * should empty element when removing innerHTML * should transition from innerHTML to children in nested el @@ -109,9 +105,6 @@ src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js src/renderers/shared/shared/__tests__/ReactUpdates-test.js * should queue mount-ready handlers across different roots * marks top-level updates -* throws in setState if the update callback is not a function -* throws in replaceState if the update callback is not a function -* throws in forceUpdate if the update callback is not a function src/renderers/shared/shared/__tests__/refs-test.js * Should increase refs with an increase in divs diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 34f94bffeee84..8ee1a4da068e1 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -577,6 +577,8 @@ src/renderers/dom/shared/__tests__/ReactDOM-test.js * should overwrite props.children with children argument * should purge the DOM cache when removing nodes * allow React.DOM factories to be called without warnings +* throws in render() if the mount callback is not a function +* throws in render() if the update callback is not a function * preserves focus src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -1431,6 +1433,9 @@ src/renderers/shared/shared/__tests__/ReactUpdates-test.js * should queue updates from during mount * calls componentWillReceiveProps setState callback properly * does not call render after a component as been deleted +* throws in setState if the update callback is not a function +* throws in replaceState if the update callback is not a function +* throws in forceUpdate if the update callback is not a function * does not update one component twice in a batch (#2410) * does not update one component twice in a batch (#6371) * unstable_batchedUpdates should return value from a callback diff --git a/src/renderers/dom/fiber/ReactDOMFiber.js b/src/renderers/dom/fiber/ReactDOMFiber.js index c5da108c5a828..001faf12a9dd3 100644 --- a/src/renderers/dom/fiber/ReactDOMFiber.js +++ b/src/renderers/dom/fiber/ReactDOMFiber.js @@ -165,7 +165,13 @@ function warnAboutUnstableUse() { warned = true; } -function renderSubtreeIntoContainer(parentComponent : ?ReactComponent, element : ReactElement, containerNode : DOMContainerElement | Document, callback: ?Function) { +function renderSubtreeIntoContainer( + parentComponent : ?ReactComponent, + element : ReactElement, + containerNode : DOMContainerElement | Document, + callback: ?Function, + callerName: ?string +) { let container : DOMContainerElement = containerNode.nodeType === DOCUMENT_NODE ? (containerNode : any).documentElement : (containerNode : any); let root; @@ -174,9 +180,10 @@ function renderSubtreeIntoContainer(parentComponent : ?ReactComponent, container : DOMContainerElement, callback: ?Function) { warnAboutUnstableUse(); - return renderSubtreeIntoContainer(null, element, container, callback); + var callerName = 'ReactDOM.render'; + return renderSubtreeIntoContainer(null, element, container, callback, callerName); }, - unstable_renderSubtreeIntoContainer(parentComponent : ReactComponent, element : ReactElement, containerNode : DOMContainerElement | Document, callback: ?Function) { + unstable_renderSubtreeIntoContainer( + parentComponent : ReactComponent, + element : ReactElement, + containerNode : DOMContainerElement | Document, + callback: ?Function + ) { invariant( parentComponent != null && ReactInstanceMap.has(parentComponent), 'parentComponent must be a valid React Component' ); - return renderSubtreeIntoContainer(parentComponent, element, containerNode, callback); + var callerName = 'ReactDOM.unstable_renderSubtreeIntoContainer'; + return renderSubtreeIntoContainer(parentComponent, element, containerNode, callback, callerName); }, unmountComponentAtNode(container : DOMContainerElement) { diff --git a/src/renderers/noop/ReactNoop.js b/src/renderers/noop/ReactNoop.js index 9f614a465ea32..1cf47c381727a 100644 --- a/src/renderers/noop/ReactNoop.js +++ b/src/renderers/noop/ReactNoop.js @@ -156,19 +156,25 @@ var ReactNoop = { // Shortcut for testing a single root render(element : ReactElement, callback: ?Function) { - ReactNoop.renderToRootWithID(element, DEFAULT_ROOT_ID, callback); + var callerName = 'ReactNoop.render'; + ReactNoop.renderToRootWithID(element, DEFAULT_ROOT_ID, callback, callerName); }, - renderToRootWithID(element : ReactElement, rootID : string, callback : ?Function) { + renderToRootWithID( + element : ReactElement, + rootID : string, + callback : ?Function, + callerName: ?string = 'ReactNoop.renderToRootWithID' + ) { if (!roots.has(rootID)) { const container = { rootID: rootID, children: [] }; rootContainers.set(rootID, container); - const root = NoopRenderer.mountContainer(element, container, null, callback); + const root = NoopRenderer.mountContainer(element, container, null, callback, callerName); roots.set(rootID, root); } else { const root = roots.get(rootID); if (root) { - NoopRenderer.updateContainer(element, root, null, callback); + NoopRenderer.updateContainer(element, root, null, callback, callerName); } } }, diff --git a/src/renderers/shared/fiber/ReactFiberClassComponent.js b/src/renderers/shared/fiber/ReactFiberClassComponent.js index 79a6a9ebc9ef0..a57ff6ec50057 100644 --- a/src/renderers/shared/fiber/ReactFiberClassComponent.js +++ b/src/renderers/shared/fiber/ReactFiberClassComponent.js @@ -66,12 +66,12 @@ module.exports = function(scheduleUpdate : (fiber: Fiber) => void) { updateQueue.isForced = true; scheduleUpdateQueue(fiber, updateQueue); }, - enqueueCallback(instance, callback) { + enqueueCallback(instance, callback, callerName) { const fiber = ReactInstanceMap.get(instance); let updateQueue = fiber.updateQueue ? fiber.updateQueue : createUpdateQueue(null); - addCallbackToQueue(updateQueue, callback); + addCallbackToQueue(updateQueue, callback, callerName); scheduleUpdateQueue(fiber, updateQueue); }, }; diff --git a/src/renderers/shared/fiber/ReactFiberReconciler.js b/src/renderers/shared/fiber/ReactFiberReconciler.js index ca2fbb7660b06..efab827cd3962 100644 --- a/src/renderers/shared/fiber/ReactFiberReconciler.js +++ b/src/renderers/shared/fiber/ReactFiberReconciler.js @@ -101,13 +101,19 @@ module.exports = function(config : HostConfig) : return { - mountContainer(element : ReactElement, containerInfo : C, parentComponent : ?ReactComponent, callback: ?Function) : OpaqueNode { + mountContainer( + element : ReactElement, + containerInfo : C, + parentComponent : ?ReactComponent, + callback: ?Function, + callerName: ?string + ) : OpaqueNode { const context = getContextForSubtree(parentComponent); const root = createFiberRoot(containerInfo, context); const container = root.current; if (callback) { const queue = createUpdateQueue(null); - addCallbackToQueue(queue, callback); + addCallbackToQueue(queue, callback, ((callerName : any) : string)); root.callbackList = queue; } // TODO: Use pending work/state instead of props. @@ -127,14 +133,20 @@ module.exports = function(config : HostConfig) : return container; }, - updateContainer(element : ReactElement, container : OpaqueNode, parentComponent : ?ReactComponent, callback: ?Function) : void { + updateContainer( + element : ReactElement, + container : OpaqueNode, + parentComponent : ?ReactComponent, + callback: ?Function, + callerName: ?string + ) : void { // TODO: If this is a nested container, this won't be the root. const root : FiberRoot = (container.stateNode : any); if (callback) { const queue = root.callbackList ? root.callbackList : createUpdateQueue(null); - addCallbackToQueue(queue, callback); + addCallbackToQueue(queue, callback, ((callerName : any) : string)); root.callbackList = queue; } root.pendingContext = getContextForSubtree(parentComponent); diff --git a/src/renderers/shared/fiber/ReactFiberUpdateQueue.js b/src/renderers/shared/fiber/ReactFiberUpdateQueue.js index 784e8b8a6b2f0..0d5a577fa564c 100644 --- a/src/renderers/shared/fiber/ReactFiberUpdateQueue.js +++ b/src/renderers/shared/fiber/ReactFiberUpdateQueue.js @@ -12,6 +12,8 @@ 'use strict'; +var invariant = require('invariant'); + type UpdateQueueNode = { partialState: any, callback: ?Function, @@ -26,6 +28,29 @@ export type UpdateQueue = UpdateQueueNode & { tail: UpdateQueueNode }; +function formatUnexpectedArgument(arg) { + var type = typeof arg; + if (type !== 'object') { + return type; + } + var displayName = arg.constructor && arg.constructor.name || type; + var keys = Object.keys(arg); + if (keys.length > 0 && keys.length < 20) { + return `${displayName} (keys: ${keys.join(', ')})`; + } + return displayName; +} + +function validateCallback(callback, callerName) { + invariant( + !callback || typeof callback === 'function', + '%s(...): Expected the last optional `callback` argument to be a ' + + 'function. Instead received: %s.', + callerName, + formatUnexpectedArgument(callback) + ); +} + exports.createUpdateQueue = function(partialState : mixed) : UpdateQueue { const queue = { partialState, @@ -56,7 +81,8 @@ function addToQueue(queue : UpdateQueue, partialState : mixed) : UpdateQueue { exports.addToQueue = addToQueue; -exports.addCallbackToQueue = function(queue : UpdateQueue, callback: Function) : UpdateQueue { +exports.addCallbackToQueue = function(queue : UpdateQueue, callback: Function, callerName: string) : UpdateQueue { + validateCallback(callback, callerName); if (queue.tail.callback) { // If the tail already as a callback, add an empty node to queue addToQueue(queue, null); @@ -115,3 +141,4 @@ exports.mergeUpdateQueue = function(queue : UpdateQueue, instance : any, prevSta } return state; }; +