Skip to content

Commit

Permalink
Handles risky callbacks on setState. Fixes facebook#8238 (facebook#8242)
Browse files Browse the repository at this point in the history
* Handles risky callbacks on setState. Fixes facebook#8238

* Updates try-catch to cover `callback` when context is not present.

* Updates code to trapErrors instead of swallowing them.

* Fixes flow errors

* Incorporates review comments

* Traps only the first error.

Removes `callbackWasCalled` and updates fiber tests.
  • Loading branch information
ankeetmaini authored and acusti committed Mar 15, 2017
1 parent a3a1bdd commit cbc47c8
Show file tree
Hide file tree
Showing 4 changed files with 68 additions and 8 deletions.
1 change: 1 addition & 0 deletions scripts/fiber/tests-passing.txt
Original file line number Diff line number Diff line change
Expand Up @@ -806,6 +806,7 @@ src/renderers/shared/fiber/__tests__/ReactIncremental-test.js
* skips will/DidUpdate when bailing unless an update was already in progress
* performs batched updates at the end of the batch
* can nest batchedUpdates
* can handle if setState callback throws

src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js
* catches render error in a boundary during mounting
Expand Down
13 changes: 11 additions & 2 deletions src/renderers/shared/fiber/ReactFiberCommitWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,11 @@ module.exports = function<T, P, I, TI, C>(
}
if (finishedWork.effectTag & Callback) {
if (finishedWork.callbackList) {
callCallbacks(finishedWork.callbackList, instance);
const callbackError = callCallbacks(finishedWork.callbackList, instance);
// since we only want to keep the first error
if (!error) {
error = callbackError;
}
finishedWork.callbackList = null;
}
}
Expand All @@ -332,10 +336,15 @@ module.exports = function<T, P, I, TI, C>(
}
case HostContainer: {
const rootFiber = finishedWork.stateNode;
let error = null;
if (rootFiber.callbackList) {
const { callbackList } = rootFiber;
rootFiber.callbackList = null;
callCallbacks(callbackList, rootFiber.current.child.stateNode);
error = callCallbacks(callbackList, rootFiber.current.child.stateNode);
}

if (error) {
trapError(rootFiber, error, false);
}
return;
}
Expand Down
18 changes: 12 additions & 6 deletions src/renderers/shared/fiber/ReactFiberUpdateQueue.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,20 +69,26 @@ exports.addCallbackToQueue = function(queue : UpdateQueue, callback: Function) :
return queue;
};

exports.callCallbacks = function(queue : UpdateQueue, context : any) {
exports.callCallbacks = function(queue : UpdateQueue, context : any) : Error | null {
let node : ?UpdateQueueNode = queue;
let error = null;
while (node) {
const callback = node.callback;
if (callback && !node.callbackWasCalled) {
node.callbackWasCalled = true;
if (typeof context !== 'undefined') {
callback.call(context);
} else {
callback();
try {
node.callbackWasCalled = true;
if (typeof context !== 'undefined') {
callback.call(context);
} else {
callback();
}
} catch (e) {
error = e;
}
}
node = node.next;
}
return error;
};

exports.mergeUpdateQueue = function(queue : UpdateQueue, instance : any, prevState : any, props : any) : any {
Expand Down
44 changes: 44 additions & 0 deletions src/renderers/shared/fiber/__tests__/ReactIncremental-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1465,4 +1465,48 @@ describe('ReactIncremental', () => {
]);
expect(instance.state.n).toEqual(4);
});

it('can handle if setState callback throws', () => {
var ops = [];
var instance;

class Foo extends React.Component {
state = { n: 0 };
render() {
instance = this;
return <div />;
}
}

ReactNoop.render(<Foo />);
ReactNoop.flush();
ops = [];

expect(instance.state.n).toEqual(0);

// first good callback
instance.setState({ n: 1 }, () => ops.push('first good callback'));
ReactNoop.flush();

// callback throws
instance.setState({ n: 2 }, () => {
throw new Error('Bail');
});
expect(() => {
ReactNoop.flush();
}).toThrow('Bail');

// should set state to 2 even if callback throws up
expect(instance.state.n).toEqual(2);

// another good callback
instance.setState({ n: 3 }, () => ops.push('second good callback'));
ReactNoop.flush();

expect(ops).toEqual([
'first good callback',
'second good callback',
]);
expect(instance.state.n).toEqual(3);
});
});

0 comments on commit cbc47c8

Please sign in to comment.