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

Fix shallow renderer not allowing hooks in forwardRef render functions #15100

Merged
merged 2 commits into from
Mar 15, 2019

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Mar 13, 2019

Fixes Invariant Violation: Hooks can only be called inside the body of a function component when shallow rendering a forwardRef component that uses hooks.

const SomeComponent = React.forwardRef((props, ref) => {
  const randomNumberRef = React.useRef({number: Math.random()});

  return (
    <div ref={ref}>
      <p>The random number is: {randomNumberRef.current.number}</p>
    </div>
  );
});

is valid when rendering as far as I can tell. At least it's a nice shorthand compared to declaring the function component with e.g. forwardedRef and then using an additional render function to pass the ref to forwardedRef.

@aweary
Copy link
Contributor

aweary commented Mar 14, 2019

The function passed to React.forwardRef isn't a function component (the second ref argument gives it away). I wouldn't expect Hooks to work here, though it appears to. @gaearon is this expected?

@eps1lon
Copy link
Collaborator Author

eps1lon commented Mar 14, 2019

The function passed to React.forwardRef isn't a function component (the second ref argument gives it away). I wouldn't expect Hooks to work here, though it appears to. @gaearon is this expected?

I wasn't quite sure about this either since it's named render wherever I found a reference. The docs also avoid using a component name in the render function: https://reactjs.org/docs/forwarding-refs.html#displaying-a-custom-name-in-devtools

I was somewhat happy about the reduced nesting in react-devtools compared to class components. Wouldn't like to see this pattern disappear since it's yet another intermediate component one would have to create just to get to the DOM node (or an imperative handle for that matter).

@eps1lon
Copy link
Collaborator Author

eps1lon commented Mar 15, 2019

Some additional context why I thought components would be fine: The docs mention that I should use a named function to improve devtools experience. However, there's no concrete example for this (only the generic myFunction). If the render function can only be a "dumb" render function (and shouldn't hold any behavior as far as I'm concerned) then why would I want a named function here?

It was very tempting to just

React.forwardRef(function Button(props, ref) {
  // some additional behavior
  return <button ref={ref} {...props} />
});

and the devtools would show ForwardRef(Button).

This is also a neat pattern if reactjs/rfcs#107 is accepted since I can just remove the forwardRef call:

-React.forwardRef(function Button(props, ref) {
+function Button(props) {
  // some additional behavior
-  return <button ref={ref} {...props} />
+  return <button {...props} />
-});
+}

If this is actually undesired behavior that function components can be passed into forwardRef I see two possible improvements:

  • add a concrete example for a named function to the docs
  • trigger a warning if a named function is passed that looks like it's intended to be a function component i.e. when the name starts with a capital letter.

@gaearon
Copy link
Collaborator

gaearon commented Mar 15, 2019

It is intentional that Hooks work inside forwardRef and memo.

@gaearon gaearon merged commit 52c870c into facebook:master Mar 15, 2019
@gaearon
Copy link
Collaborator

gaearon commented Mar 15, 2019

Thanks

@eps1lon eps1lon deleted the fix/shallow/forwardRef-hooks branch March 15, 2019 15:40
@oliviertassinari
Copy link
Contributor

oliviertassinari commented Mar 15, 2019

It is intentional that Hooks work inside forwardRef and memo.

Time to party 💃 https://twitter.com/olivtassinari/status/1106224970043219970.

gaearon pushed a commit to gaearon/react that referenced this pull request Mar 22, 2019
facebook#15100)

* test: Add test for shallow + forwardRef + hook

* fix(react-test-renderer): shallow forwardRef hooks
@gaearon gaearon mentioned this pull request Mar 22, 2019
@gaearon
Copy link
Collaborator

gaearon commented Mar 22, 2019

Fixed in 16.8.5.

This was referenced Oct 26, 2019
NMinhNguyen referenced this pull request in enzymejs/react-shallow-renderer Jan 29, 2020
…s (#15100)

* test: Add test for shallow + forwardRef + hook

* fix(react-test-renderer): shallow forwardRef hooks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants