Improve error message thrown in Fiber with multiple copies of React

[flarnie]

what is the change?:

• Added warning in the place where this error is thrown in Fiber, to
  get parity with older versions of React.
• Updated docs to mention new error message as well as old one.

I started to write a new docs page for the new error, and realized the
content was the same as the old one. So then I just updated the existing
error page.

why make this change?:
We want to avoid confusion when this error is thrown from React v16.

test plan:

• yarn test src/renderers/__tests__/multiple-copies-of-react-test.js
• REACT_JEST_USE_FIBER=1 yarn run test src/renderers/__tests__/multiple-copies-of-react-test.js
• manually inspected docs page
• manually tested in a CRA to trigger the error message

issue:
Fixes #9962
Related to #8854

[sophiebits]

Would this invariant make more sense as an else to this if?

react/src/renderers/shared/fiber/ReactChildFiber.js

Line 124 in 2e2f503

if (element._owner) {

That way only that one piece of code needs to care about string refs.

[flarnie]

I think that makes sense @spicyj - if we put the 'invariant' there then we could even use basically the same error message and doc page, right?

[sophiebits]

Yeah.

I would tweak it a bit: "Expected ref to be a function" is a little confusing if you pass a string ref because it requires the reader to understand that React converts the string to a function behind the scenes, which is an implementation detail. Something like "Element ref was specified as a string (myRef) but no owner was set." could be better. In the case that it's neither string nor function, you could say "Expected ref to be a function or a string".

[gaearon]

Maybe also let's not include commitAttachRef(...) there? It's not very useful to us since there's only one such invariant, and it's confusing to the user since it shows an internal method name as if they were expected to know what it means.

[flarnie]

Ah - I meant we could reuse the warning wording and docs from the old warning that gets thrown in 15.* - https://facebook.github.io/react/warnings/refs-must-have-owner.html

But I think you are right, we should give a specific error for this case, and probably a new docs page similar to https://facebook.github.io/react/warnings/refs-must-have-owner.html

I spent a while trying to write a test for this, and wasn't able to reproduce the error in the test environment. I was trying to use 'jestResetModules' to load and use a second copy of React, and it seemed to work, but the error wasn't triggered.

This is what I tried: https://gist.github.com/flarnie/46f744f6981f1f35861ef4cdeba67af1

Since we didn't have a test for the old warning (Refs must have owner) we could merge this without a test.

Writing a fixture that reproduces the error seems easier, but I don't want to commit to the overhead of checking it manually.

[flarnie]

I can't find any way to trigger the case where 'In the case that it's neither string nor function, you could say "Expected ref to be a function or a string".' so I'm not putting too much thought into that message, but I guess it makes sense to add that invariant since we do expect it to be a string or a function.

[sophiebits]

What happens if you do ref={17} or ref={{abc: true}}?

[flarnie]

It doesn't complain for ref={17}, ref={{abc: true}}. The only thing I found to pass to ref that throws is ref={Function} and I think that is bad syntax, not really worried about someone doing that.

[flarnie]

And now it has a test! Thank youuuuuu @gaearon 🏎️

[sophiebits]

Why do you end up splitting out the tests between stack and fiber?

[flarnie]

I thought it would be more clear that way - looking now for patterns that are used currently.

[flarnie]

I'll update it to all be in one it(... assertion.

[sophiebits]

I guess I am confused why there needs to be a branch at all. Is the behavior not the same?

[flarnie]

For the record, we talked more about why there needs to be a different branch for 'fiber' and 'stack' versions of this behavior; the new warning is in fiber only, and the old warning shows up if you are using stack.

The old warning will go away when we delete stack, but for now the test is covering both cases.