Skip to content

[Fiber] Validate that update callback is a function #8639

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gaearon merged 1 commit into from
Dec 27, 2016
Merged

[Fiber] Validate that update callback is a function #8639

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions scripts/fiber/tests-failing.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,9 +86,6 @@ src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js

src/renderers/shared/shared/__tests__/ReactUpdates-test.js
* 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
3 changes: 3 additions & 0 deletions scripts/fiber/tests-passing.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1564,6 +1564,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
34 changes: 34 additions & 0 deletions src/renderers/shared/fiber/ReactFiberClassComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,31 @@ var invariant = require('invariant');

const isArray = Array.isArray;

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) {
if (typeof callback !== 'function') {
invariant(
false,
'%s(...): Expected the last optional `callback` argument to be a ' +
'function. Instead received: %s.',
callerName,
formatUnexpectedArgument(callback)
);
}
}
Copy link
Contributor

@aweary aweary Dec 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense for this to live in ReactFiberUpdateQueue like it does in stack with ReactUpdateQueue?

Copy link
Collaborator Author

@gaearon gaearon Dec 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would the benefit be? They're not direct equivalents, just happen to be called similarly.

Copy link
Contributor

@aweary aweary Dec 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if they're not direct equivalents, if validateCallback is meant to validate a callback specifically for enqueued callbacks it would make sense for it to live alongside the rest of the queue logic. Not a big deal though.


module.exports = function(
scheduleUpdate : (fiber : Fiber, priorityLevel : PriorityLevel) => void,
getPriorityContext : () => PriorityLevel,
Expand All @@ -42,18 +67,27 @@ module.exports = function(
const updater = {
isMounted,
enqueueSetState(instance, partialState, callback) {
if (callback) {
validateCallback(callback, 'setState');
}
const fiber = ReactInstanceMap.get(instance);
const priorityLevel = getPriorityContext();
addUpdate(fiber, partialState, callback || null, priorityLevel);
scheduleUpdate(fiber, priorityLevel);
},
enqueueReplaceState(instance, state, callback) {
if (callback) {
validateCallback(callback, 'replaceState');
}
const fiber = ReactInstanceMap.get(instance);
const priorityLevel = getPriorityContext();
addReplaceUpdate(fiber, state, callback || null, priorityLevel);
scheduleUpdate(fiber, priorityLevel);
},
enqueueForceUpdate(instance, callback) {
if (callback) {
validateCallback(callback, 'forceUpdate');
}
const fiber = ReactInstanceMap.get(instance);
const priorityLevel = getPriorityContext();
addForceUpdate(fiber, callback || null, priorityLevel);
Expand Down