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

useImperativeHandle should warn when second arg isn't a function #14629

Closed
lorenzomigliorero opened this issue Jan 18, 2019 · 11 comments · Fixed by #14647
Closed

useImperativeHandle should warn when second arg isn't a function #14629

lorenzomigliorero opened this issue Jan 18, 2019 · 11 comments · Fixed by #14647

Comments

@lorenzomigliorero
Copy link

lorenzomigliorero commented Jan 18, 2019

I've noticed a strange bug with the react redux forwardRef opt-in.
If i use it with a connected class component, everything is ok:

const MyComponent = class Test extends React.Component {
  foo = () => console.log("Print foo from Test component");
  render() {
    return null;
  }
};

const ConnectedComponent = connect(
  null,
  null,
  null,
  { forwardRef: true }
)(MyComponent);

const store = createStore(() => {});

function App() {
  return (
    <Provider store={store}>
      <ConnectedComponent
        ref={ref => {
          if (ref) ref.foo();
        }}
      />
    </Provider>
  );
}

const rootElement = document.getElementById("root");
ReactDOM.render(<App />, rootElement);

If i use it with a connected functional component that use forwardRef with useImperativeHandle, i obtain a strange error: create is not a function in commitHookEffectList react-dom method.

const MyComponent = React.forwardRef((props, ref) => {
  useImperativeHandle(ref, {
    foo: () => console.log("Print foo from Test component")
  });

  return null;
});

const ConnectedComponent = connect(
  null,
  null,
  null,
  { forwardRef: true }
)(MyComponent);

const store = createStore(() => {});

function App() {
  return (
    <Provider store={store}>
      <ConnectedComponent
        ref={ref => {
          if (ref) ref.foo();
        }}
      />
    </Provider>
  );
}

const rootElement = document.getElementById("root");
ReactDOM.render(<App />, rootElement);

I create a codepen to reproduce the issue: https://codesandbox.io/s/r7rpml460o

PS: Sorry for the cors error, but i don't find the way to add react@nextas cdn

@hamlim
Copy link
Contributor

hamlim commented Jan 18, 2019

I believe the issue here is that useImperativeHandle expects the second argument to be a function that returns the reference:

// before (your sample code)
useImperativeHandle(ref, {
  foo: () => console.log("Print foo from Test component")
});
// after
useImperativeHandle(ref, () => ({
  foo: () => console.log("Print foo from Test component")
}));

See: https://reactjs.org/docs/hooks-reference.html#useimperativehandle for more information about the API of the hook.

Maybe the error thrown could be more clear?

@lorenzomigliorero
Copy link
Author

lorenzomigliorero commented Jan 18, 2019

You're right, useImperativeHandle accept a function as second argument.
A warning on wrong argument type could be a good idea.

@gaearon gaearon changed the title forwardRef does not work with useImperativeHandle useImperativeHandle should warn when second arg isn't a function Jan 18, 2019
@Aberman12
Copy link

Ill take this issue on

@gaearon
Copy link
Collaborator

gaearon commented Jan 18, 2019

Sure

@AhmedElywa
Copy link

i get TypeError: React.useImperativeMethods is not a function in 16.8.0-alpha.1 but when back to 16.7.0-alpha.2 not get this error

@hamlim
Copy link
Contributor

hamlim commented Jan 21, 2019

@AhmedElywa The hook was renamed in this PR: #14565

Most likely between the 16.7 alpha and 16.8 alpha

@gaearon
Copy link
Collaborator

gaearon commented Jan 21, 2019

Yeah, it was renamed.

@AhmedElywa
Copy link

yes it work good after rename it where i can get changes between alpha version ?

@gaearon
Copy link
Collaborator

gaearon commented Jan 21, 2019

Commit log. We don't write changelogs for alphas.

@gaearon
Copy link
Collaborator

gaearon commented Jan 21, 2019

@Aberman12 I'm taking this in #14647 since we want to get Hooks out sooner.

@Aberman12
Copy link

alright np

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants