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.

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

Jump to bottom

Improve error message thrown in Fiber with multiple copies of React #10151

Merged
merged 7 commits into from
Jul 13, 2017
Merged

Improve error message thrown in Fiber with multiple copies of React #10151

merged 7 commits into from
Jul 13, 2017

Commits on Jul 13, 2017

  1. WIP Improve error message thrown in Fiber with multiple copies of React 

    **what is the change?:**
Adding an 'invariant' with detailed error message for the problem that
occurs when you load two copies of React with the Fiber reconciler.

WIP:
 - Is there any other likely cause for this error besides two copies of
   React?
 - How can we make the message more clear?

Still TODO:
 - Write a unit test
 - Write a documentation page and create the link to the page, similar
   to https://facebook.github.io/react/warnings/refs-must-have-owner.html

It would also be nice to have a page with instructions on how to fix
common cases of accidental double-loading of React, but we can open a
separate issue on that and let the community do it.

**why make this change?:**
This error comes up relatively often and we want to make things clear
when it happens in v16.0+

**test plan:**
WIP

**issue:**
Fixes facebook#9962
    @flarnie
    flarnie committed Jul 13, 2017
    Configuration menu
    Copy the full SHA
    b23d326 View commit details
    Browse the repository at this point in the history

  2. Add improved warning and docs for 'refs must have owner' in Fiber 

    **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:**
- manually inspected docs page
- manually tested in a CRA to trigger the error message

(Flarnie will insert screenshots)

**issue:**
Fixes facebook#9962
Related to facebook#8854
    @flarnie
    flarnie committed Jul 13, 2017
    Configuration menu
    Copy the full SHA
    8cb9df3 View commit details
    Browse the repository at this point in the history

  3. Add test for the informative warning around multiple react copies 

    @gaearon debugged the test for this and now it works!!!!!!!!!!!!!!!

!!!!!!!!!!!!!!!!!!!!!!!!!!!!! :)

**what is the change?:**
We now test for the warning that the previous commits add in Fiber, and
also test for the old warning in the stack reconciler.

**why make this change?:**
This is an especially important warning, and we want to know that it
will fire correctly.

**test plan:**
`yarn test src/renderers/__tests__/multiple-copies-of-react-test.js`
`REACT_DOM_JEST_USE_FIBER=1 yarn test src/renderers/__tests__/multiple-copies-of-react-test.js`
    @flarnie
    flarnie committed Jul 13, 2017
    Configuration menu
    Copy the full SHA
    3563c6f View commit details
    Browse the repository at this point in the history

  4. Fix up test for 'multiple copies of react' 

    **what is the change?:**
refactor test for 'multiple copies of React' to be simpler and remove
some copypasta
    @flarnie
    flarnie committed Jul 13, 2017
    Configuration menu
    Copy the full SHA
    7206c78 View commit details
    Browse the repository at this point in the history

  5. run prettier

    @flarnie
    flarnie committed Jul 13, 2017
    Configuration menu
    Copy the full SHA
    59ef3d6 View commit details
    Browse the repository at this point in the history

  6. Fix conditionals in 'multiple copies of react' test 

    **what is the change?:**
When moving the 'fiber' and 'non-fiber' conditions from two assertions
into one, we copy pasted the wrong message into the 'fiber' condition.

This wasn't caught because we were using an outdated name for the
'fiber' constant when running the tests locally with fiber enabled.

This fixes the copy-paste error and we now are actually running the
tests with fiber enabled locally.
    @flarnie
    flarnie committed Jul 13, 2017
    Configuration menu
    Copy the full SHA
    7318403 View commit details
    Browse the repository at this point in the history

  7. Run scripts/fiber/record-tests

    @flarnie
    flarnie committed Jul 13, 2017
    Configuration menu
    Copy the full SHA
    34f0b4d View commit details
    Browse the repository at this point in the history