Skip to content
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.

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

Warn about incorrect use of useImperativeHandle() #14647

Merged
merged 1 commit into from Jan 21, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 14 additions & 0 deletions packages/react-reconciler/src/ReactFiberHooks.js
Expand Up @@ -674,6 +674,12 @@ export function useImperativeHandle<T>(
): void {
if (__DEV__) {
currentHookNameInDev = 'useImperativeHandle';
warning(
typeof create === 'function',
'Expected useImperativeHandle() second argument to be a function ' +
'that creates a handle. Instead received: %s.',
create !== null ? typeof create : 'null',
);
}
// TODO: If deps are provided, should we skip comparing the ref itself?
const nextDeps =
Expand All @@ -690,6 +696,14 @@ export function useImperativeHandle<T>(
return () => refCallback(null);
} else if (ref !== null && ref !== undefined) {
const refObject = ref;
if (__DEV__) {
warning(
refObject.hasOwnProperty('current'),
'Expected useImperativeHandle() first argument to either be a ' +
'ref callback or React.createRef() object. Instead received: %s.',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(Note: callback is handled by previous if-clause so we don't need to check for that)

'an object with keys {' + Object.keys(refObject).join(', ') + '}',
);
}
const inst = create();
refObject.current = inst;
return () => {
Expand Down
13 changes: 13 additions & 0 deletions packages/react-reconciler/src/ReactFiberScheduler.js
Expand Up @@ -489,6 +489,9 @@ function commitAllLifeCycles(
}
}
while (nextEffect !== null) {
if (__DEV__) {
setCurrentFiber(nextEffect);
}
const effectTag = nextEffect.effectTag;

if (effectTag & (Update | Callback)) {
Expand All @@ -513,6 +516,9 @@ function commitAllLifeCycles(

nextEffect = nextEffect.nextEffect;
}
if (__DEV__) {
resetCurrentFiber();
}
}

function commitPassiveEffects(root: FiberRoot, firstEffect: Fiber): void {
Expand All @@ -526,6 +532,10 @@ function commitPassiveEffects(root: FiberRoot, firstEffect: Fiber): void {

let effect = firstEffect;
do {
if (__DEV__) {
setCurrentFiber(effect);
}

if (effect.effectTag & Passive) {
let didError = false;
let error;
Expand All @@ -549,6 +559,9 @@ function commitPassiveEffects(root: FiberRoot, firstEffect: Fiber): void {
}
effect = effect.nextEffect;
} while (effect !== null);
if (__DEV__) {
resetCurrentFiber();
}

isRendering = previousIsRendering;

Expand Down
Expand Up @@ -569,6 +569,45 @@ describe('ReactHooks', () => {
]);
});

it('warns for bad useImperativeHandle first arg', () => {
const {useImperativeHandle} = React;
function App() {
useImperativeHandle({
focus() {},
});
return null;
}

expect(() => {
expect(() => {
ReactTestRenderer.create(<App />);
}).toThrow('create is not a function');
}).toWarnDev([
'Expected useImperativeHandle() first argument to either be a ' +
'ref callback or React.createRef() object. ' +
'Instead received: an object with keys {focus}.',
'Expected useImperativeHandle() second argument to be a function ' +
'that creates a handle. Instead received: undefined.',
]);
});

it('warns for bad useImperativeHandle second arg', () => {
const {useImperativeHandle} = React;
const App = React.forwardRef((props, ref) => {
useImperativeHandle(ref, {
focus() {},
});
return null;
});

expect(() => {
ReactTestRenderer.create(<App />);
}).toWarnDev([
'Expected useImperativeHandle() second argument to be a function ' +
'that creates a handle. Instead received: object.',
]);
});

// https://github.com/facebook/react/issues/14022
it('works with ReactDOMServer calls inside a component', () => {
const {useState} = React;
Expand Down