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

String Refs Without An Owner Should Fail Gracefully #9962

Closed
sebmarkbage opened this Issue Jun 14, 2017 · 9 comments

Comments

Projects
None yet
4 participants
@sebmarkbage
Member

sebmarkbage commented Jun 14, 2017

Occurs in Fiber with multiple instances of the React package (which can't share current owner).

@gaearon gaearon added this to the 16.0 milestone Jul 11, 2017

@gaearon gaearon referenced this issue Jul 11, 2017

Closed

React 16 Umbrella ☂️ (and 15.5) #8854

117 of 120 tasks complete

@flarnie flarnie self-assigned this Jul 11, 2017

@flarnie

This comment has been minimized.

Show comment
Hide comment
@flarnie

flarnie Jul 11, 2017

Contributor

Context:

We theorize that this may be triggered by different versions of React being required for libraries depending on React. As libraries update to support version 16, this may come up more commonly than in the past.

At the very least, it seems common enough now to be worth fixing, with 300+ issues open across various libraries: https://github.com/search?q=%22Refs+must+have%22&type=Issues&utf8=%E2%9C%93

Folks on this issue are helping out with more info about how to reproduce and debug this state: facebook/draft-js#296

Plan:
MVP:

  • Add a warning for any time that multiple copies of React are loaded.
  • Link to documentation about how/why this might happen and how to fix it.

Bonus:

  • Indicate multiple copies of React with the devtools icon
  • (?) Add a work-around in the code itself such that React itself will work ok even when you have loaded multiple copies. -> We might not want to do this, since it allows users to continue in a "broken" state.
Contributor

flarnie commented Jul 11, 2017

Context:

We theorize that this may be triggered by different versions of React being required for libraries depending on React. As libraries update to support version 16, this may come up more commonly than in the past.

At the very least, it seems common enough now to be worth fixing, with 300+ issues open across various libraries: https://github.com/search?q=%22Refs+must+have%22&type=Issues&utf8=%E2%9C%93

Folks on this issue are helping out with more info about how to reproduce and debug this state: facebook/draft-js#296

Plan:
MVP:

  • Add a warning for any time that multiple copies of React are loaded.
  • Link to documentation about how/why this might happen and how to fix it.

Bonus:

  • Indicate multiple copies of React with the devtools icon
  • (?) Add a work-around in the code itself such that React itself will work ok even when you have loaded multiple copies. -> We might not want to do this, since it allows users to continue in a "broken" state.
@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 11, 2017

Member

At the very least, it seems common enough now to be worth fixing, with 300+ issues open across various libraries:

To be clear, I think this issue is not about completely fixing this error, but about making the error message comprehensible in Fiber.

Stack produced an error message that linked to the error page. Those issues you linked to are the ones that already have a legible error message. But Fiber doesn't show this message at all—it just crashes with a TypeError. I believe this issue was about Fiber parity with Stack in failing gracefully rather than crashing in internals. So we'd need to add an invariant with this error message to Fiber code (since now it only exists in Stack code).

But surely we could make the message better by including debugging instructions, and improve error page. Adding npm ls react suggestion to the error message (and the page) and making the message more prominently about duplicate React would very likely be helpful.

We can also take a stab at, if not fixing, then reducing how common it is, by extracting a separate package for shared mutable state. But I think that would be worth tracking in a separate issue, and it doesn’t block 16 beta.

Apologies if this is what you meant already! Just want to make sure we’re on the same page re: action item here. Making this more resilient would be great but seems like more work than just putting an invariant in.

Member

gaearon commented Jul 11, 2017

At the very least, it seems common enough now to be worth fixing, with 300+ issues open across various libraries:

To be clear, I think this issue is not about completely fixing this error, but about making the error message comprehensible in Fiber.

Stack produced an error message that linked to the error page. Those issues you linked to are the ones that already have a legible error message. But Fiber doesn't show this message at all—it just crashes with a TypeError. I believe this issue was about Fiber parity with Stack in failing gracefully rather than crashing in internals. So we'd need to add an invariant with this error message to Fiber code (since now it only exists in Stack code).

But surely we could make the message better by including debugging instructions, and improve error page. Adding npm ls react suggestion to the error message (and the page) and making the message more prominently about duplicate React would very likely be helpful.

We can also take a stab at, if not fixing, then reducing how common it is, by extracting a separate package for shared mutable state. But I think that would be worth tracking in a separate issue, and it doesn’t block 16 beta.

Apologies if this is what you meant already! Just want to make sure we’re on the same page re: action item here. Making this more resilient would be great but seems like more work than just putting an invariant in.

@flarnie

This comment has been minimized.

Show comment
Hide comment
@flarnie

flarnie Jul 12, 2017

Contributor

Thanks for clarifying this -

"But Fiber doesn't show this message at all—it just crashes with a TypeError."

I did notice that, you're right that fixing that is the main blocker.

We can improve that error message to make it on par with the "Refs must have owner" warning. Maybe that's an acceptable first step.

It seems to me that reaching that point is just a symptom of the larger problem. We don't support having two versions of React running, and it's usually done by mistake when it happens. If we add a warning and better docs around that I think that will help even more, but that also is not needed for the 16 beta release so I can open a separate issue for that.

Edit: for context, here is what it looks like in Fiber if you load multiple copies of React -

screen shot 2017-07-11 at 5 07 03 pm

Contributor

flarnie commented Jul 12, 2017

Thanks for clarifying this -

"But Fiber doesn't show this message at all—it just crashes with a TypeError."

I did notice that, you're right that fixing that is the main blocker.

We can improve that error message to make it on par with the "Refs must have owner" warning. Maybe that's an acceptable first step.

It seems to me that reaching that point is just a symptom of the larger problem. We don't support having two versions of React running, and it's usually done by mistake when it happens. If we add a warning and better docs around that I think that will help even more, but that also is not needed for the 16 beta release so I can open a separate issue for that.

Edit: for context, here is what it looks like in Fiber if you load multiple copies of React -

screen shot 2017-07-11 at 5 07 03 pm

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 12, 2017

Member

I think adding a warning was discussed before (#2402) but there was no clear way of how we’d go about it. There are legitimate cases for independent Reacts on the same page (e.g. third party widget that you have no control over), and it works if they don’t “cross” with each other.

I think doing this in DevTools is a great idea though.

Member

gaearon commented Jul 12, 2017

I think adding a warning was discussed before (#2402) but there was no clear way of how we’d go about it. There are legitimate cases for independent Reacts on the same page (e.g. third party widget that you have no control over), and it works if they don’t “cross” with each other.

I think doing this in DevTools is a great idea though.

@flarnie

This comment has been minimized.

Show comment
Hide comment
@flarnie

flarnie Jul 12, 2017

Contributor

Thanks for linking to that issue - that is great context. Looks like there was a warning, then it was removed, then it was added again, then removed at some point.

Was it a pain point for people to have the warning? I don't open a new issue if we've already decided that we generally support loading two copies of React and are not going to warn for it.

Contributor

flarnie commented Jul 12, 2017

Thanks for linking to that issue - that is great context. Looks like there was a warning, then it was removed, then it was added again, then removed at some point.

Was it a pain point for people to have the warning? I don't open a new issue if we've already decided that we generally support loading two copies of React and are not going to warn for it.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jul 12, 2017

Member

I think the best explanation is in #3580 (comment). There are legitimate cases for when you’d want to do it. I agree we need to only warn for patterns that are clearly unsupported. DevTools seems like best of both worlds: it lets you know something is suboptimal, but doesn’t spam your console.

Member

gaearon commented Jul 12, 2017

I think the best explanation is in #3580 (comment). There are legitimate cases for when you’d want to do it. I agree we need to only warn for patterns that are clearly unsupported. DevTools seems like best of both worlds: it lets you know something is suboptimal, but doesn’t spam your console.

flarnie added a commit to flarnie/react that referenced this issue Jul 12, 2017

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 #9962

flarnie added a commit to flarnie/react that referenced this issue Jul 12, 2017

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 #9962

flarnie added a commit to flarnie/react that referenced this issue Jul 12, 2017

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 #9962
Related to #8854

flarnie added a commit to flarnie/react that referenced this issue Jul 13, 2017

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 #9962

flarnie added a commit to flarnie/react that referenced this issue Jul 13, 2017

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 #9962
Related to #8854

@flarnie flarnie closed this in #10151 Jul 13, 2017

flarnie added a commit that referenced this issue Jul 13, 2017

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

* 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 #9962

* 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 #9962
Related to #8854

* 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`

* 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

* run prettier

* 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.

* Run scripts/fiber/record-tests

@bvaughn bvaughn referenced this issue Aug 1, 2017

Closed

React 16 RC #10294

@nareshbhatia

This comment has been minimized.

Show comment
Hide comment
@nareshbhatia

nareshbhatia Apr 6, 2018

I am running into this issue with my app created with create-react-app. I am pulling in a library that uses react and also using Storybook from tests. I eliminated react from both those and I am down to one copy as indicated by npm ls react:

myapp@0.1.0 /projects/myrepo/packages/myapp
└── react@16.3.0

Is there a way to figure out where react thinks the second copy is being loaded from?

nareshbhatia commented Apr 6, 2018

I am running into this issue with my app created with create-react-app. I am pulling in a library that uses react and also using Storybook from tests. I eliminated react from both those and I am down to one copy as indicated by npm ls react:

myapp@0.1.0 /projects/myrepo/packages/myapp
└── react@16.3.0

Is there a way to figure out where react thinks the second copy is being loaded from?

@nareshbhatia

This comment has been minimized.

Show comment
Hide comment
@nareshbhatia

nareshbhatia Apr 6, 2018

Well, after a full day of trial-and-error, I figured it out. The second copy was coming from Storybook. The moment I removed it, everything started working. Wish React just told me where the second copy was coming from!

nareshbhatia commented Apr 6, 2018

Well, after a full day of trial-and-error, I figured it out. The second copy was coming from Storybook. The moment I removed it, everything started working. Wish React just told me where the second copy was coming from!

@nareshbhatia

This comment has been minimized.

Show comment
Hide comment
@nareshbhatia

nareshbhatia Apr 7, 2018

To resolve the issue, I tried to restructure my repo and move storybook to a dev dependency - hoping that it will prevent the second copy from being loaded. Unfortunately it did not solve the issue. I have created a reproducible example here: https://github.com/nareshbhatia/react-multiple-copies-issue. It contains my library in one package and an app in another package (created with create-react-app). Would really appreciate if someone could explain what is going on. How is the second copy being loaded?

nareshbhatia commented Apr 7, 2018

To resolve the issue, I tried to restructure my repo and move storybook to a dev dependency - hoping that it will prevent the second copy from being loaded. Unfortunately it did not solve the issue. I have created a reproducible example here: https://github.com/nareshbhatia/react-multiple-copies-issue. It contains my library in one package and an app in another package (created with create-react-app). Would really appreciate if someone could explain what is going on. How is the second copy being loaded?

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