Skip to content

Commit

Permalink
Fix Fiber tests for update callback warnings
Browse files Browse the repository at this point in the history
  • Loading branch information
koba04 committed Nov 29, 2016
1 parent c740345 commit f141f18
Show file tree
Hide file tree
Showing 7 changed files with 81 additions and 24 deletions.
7 changes: 0 additions & 7 deletions scripts/fiber/tests-failing.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
5 changes: 5 additions & 0 deletions scripts/fiber/tests-passing.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
26 changes: 20 additions & 6 deletions src/renderers/dom/fiber/ReactDOMFiber.js
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,13 @@ function warnAboutUnstableUse() {
warned = true;
}

function renderSubtreeIntoContainer(parentComponent : ?ReactComponent<any, any, any>, element : ReactElement<any>, containerNode : DOMContainerElement | Document, callback: ?Function) {
function renderSubtreeIntoContainer(
parentComponent : ?ReactComponent<any, any, any>,
element : ReactElement<any>,
containerNode : DOMContainerElement | Document,
callback: ?Function,
callerName: ?string
) {
let container : DOMContainerElement =
containerNode.nodeType === DOCUMENT_NODE ? (containerNode : any).documentElement : (containerNode : any);
let root;
Expand All @@ -174,9 +180,10 @@ function renderSubtreeIntoContainer(parentComponent : ?ReactComponent<any, any,
while (container.lastChild) {
container.removeChild(container.lastChild);
}
root = container._reactRootContainer = DOMRenderer.mountContainer(element, container, parentComponent, callback);
root = container._reactRootContainer =
DOMRenderer.mountContainer(element, container, parentComponent, callback, callerName);
} else {
DOMRenderer.updateContainer(element, root = container._reactRootContainer, parentComponent, callback);
DOMRenderer.updateContainer(element, root = container._reactRootContainer, parentComponent, callback, callerName);
}
return DOMRenderer.getPublicRootInstance(root);
}
Expand All @@ -185,15 +192,22 @@ var ReactDOM = {

render(element : ReactElement<any>, 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<any, any, any>, element : ReactElement<any>, containerNode : DOMContainerElement | Document, callback: ?Function) {
unstable_renderSubtreeIntoContainer(
parentComponent : ReactComponent<any, any, any>,
element : ReactElement<any>,
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) {
Expand Down
14 changes: 10 additions & 4 deletions src/renderers/noop/ReactNoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -156,19 +156,25 @@ var ReactNoop = {

// Shortcut for testing a single root
render(element : ReactElement<any>, callback: ?Function) {
ReactNoop.renderToRootWithID(element, DEFAULT_ROOT_ID, callback);
var callerName = 'ReactNoop.render';
ReactNoop.renderToRootWithID(element, DEFAULT_ROOT_ID, callback, callerName);
},

renderToRootWithID(element : ReactElement<any>, rootID : string, callback : ?Function) {
renderToRootWithID(
element : ReactElement<any>,
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);
}
}
},
Expand Down
4 changes: 2 additions & 2 deletions src/renderers/shared/fiber/ReactFiberClassComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
},
};
Expand Down
20 changes: 16 additions & 4 deletions src/renderers/shared/fiber/ReactFiberReconciler.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,13 +101,19 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) :

return {

mountContainer(element : ReactElement<any>, containerInfo : C, parentComponent : ?ReactComponent<any, any, any>, callback: ?Function) : OpaqueNode {
mountContainer(
element : ReactElement<any>,
containerInfo : C,
parentComponent : ?ReactComponent<any, any, any>,
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.
Expand All @@ -127,14 +133,20 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) :
return container;
},

updateContainer(element : ReactElement<any>, container : OpaqueNode, parentComponent : ?ReactComponent<any, any, any>, callback: ?Function) : void {
updateContainer(
element : ReactElement<any>,
container : OpaqueNode,
parentComponent : ?ReactComponent<any, any, any>,
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);
Expand Down
29 changes: 28 additions & 1 deletion src/renderers/shared/fiber/ReactFiberUpdateQueue.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@

'use strict';

var invariant = require('invariant');

type UpdateQueueNode = {
partialState: any,
callback: ?Function,
Expand All @@ -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,
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -115,3 +141,4 @@ exports.mergeUpdateQueue = function(queue : UpdateQueue, instance : any, prevSta
}
return state;
};

0 comments on commit f141f18

Please sign in to comment.