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

An exception is thrown when using Mobx6 and react18 in development mode #3492

Closed
guochengcheng04 opened this issue Aug 10, 2022 · 19 comments · Fixed by #3590 or #3661 · May be fixed by #3515
Closed

An exception is thrown when using Mobx6 and react18 in development mode #3492

guochengcheng04 opened this issue Aug 10, 2022 · 19 comments · Fixed by #3590 or #3661 · May be fixed by #3515
Labels

Comments

@guochengcheng04
Copy link

Intended outcome:

When I use React18's new rendering API ReactDOM.createRoot().render, I get a lot of warnings. It should be guaranteed to be consistent with the previous behavior, run normally, and not throw warnings

warning message: react_devtools_backend.js:3973 Warning: Can't perform a React state update on a component that hasn't mounted yet. This indicates that you have a side-effect in your render function that asynchronously later calls tries to update the component. Move this work to useEffect instead.

Actual outcome:

I get a lot of warnings, but can't find the information I have in it, like below

Versions

mobx@^6.6.1 mobx-react@^7.5.1

Dzdw92s8dG

@kubk
Copy link
Collaborator

kubk commented Aug 10, 2022

Please create a runnable reproduction using codesandbox. Here is an example of Mobx + React 18 that shows no warnings: https://codesandbox.io/s/vigilant-hellman-3mjsty?file=/index.js

@guochengcheng04
Copy link
Author

Please create a runnable reproduction using codesandbox. Here is an example of Mobx + React 18 that shows no warnings: https://codesandbox.io/s/vigilant-hellman-3mjsty?file=/index.js

@kubk hello, this is the demo code link. https://codesandbox.io/s/mobx6-and-react18-2rx40e?file=/index.js

@guochengcheng04
Copy link
Author

Please create a runnable reproduction using codesandbox. Here is an example of Mobx + React 18 that shows no warnings: https://codesandbox.io/s/vigilant-hellman-3mjsty?file=/index.js

@kubk hello, this is the demo code link. https://codesandbox.io/s/mobx6-and-react18-2rx40e?file=/index.js

and in this case, this problem can also be reproduced by clicking the button, https://codesandbox.io/s/mobx6-and-react18-2-gu58ts?file=/CompA.js

@mweststrate
Copy link
Member

Is there any progress on this please?

@guochengcheng04 please remove this comment. It is rude, and completely uncalled for within four hours. This software is provided to you as-is, and people are helping out here on a voluntarily basis, to help you with the problems you are probably paid to solve. So please refrain from such comments, and if you want to increase your chance on follow up, consider instead contributing to the community yourself, by helping out others with their issues.

@guochengcheng04
Copy link
Author

Is there any progress on this please?

@guochengcheng04 please remove this comment. It is rude, and completely uncalled for within four hours. This software is provided to you as-is, and people are helping out here on a voluntarily basis, to help you with the problems you are probably paid to solve. So please refrain from such comments, and if you want to increase your chance on follow up, consider instead contributing to the community yourself, by helping out others with their issues.

sorry, my bad. because this problem is stable and recurring, and it is important

@urugator
Copy link
Collaborator

@mweststrate We will have to handle reaction disposal of class components similary to functional ones. Apparently react can now decide to run render and throw the component instance away without running lifecycle methods. It does it when using Suspense in render.

@guochengcheng04
Copy link
Author

@mweststrate We will have to handle reaction disposal of class components similary to functional ones. Apparently react can now decide to run render and throw the component instance away without running lifecycle methods. It does it when using Suspense in render.

@urugator If you can provide me with some ideas, I think I'd be happy to solve it

@urugator
Copy link
Collaborator

urugator commented Aug 12, 2022

@guochengcheng04 If you mean to PR, then you will have to take a look at:
https://github.com/mobxjs/mobx/blob/main/packages/mobx-react-lite/src/utils/reactionCleanupTracking.ts
and how it's being used in useObserver:

addReactionToTrack,
IReactionTracking,
recordReactionAsCommitted

Then the goal is to make use of these in a similar fashion in observerClass:
https://github.com/mobxjs/mobx/blob/main/packages/mobx-react/src/observerClass.ts

If you mean what you can do in your code base, then you can rewrite components using Suspense into functional ones or instead of using Suspense directly, you can wrap it into a simple wrapper and use it instead:

function SuspenseWorkaround(...props) {
  return <Suspense {...props}></Suspense>
}

const root = createRoot(document.getElementById("root"));
root.render(
  <SuspenseWorkaround fallback={<div id="outer-loading" />}>
    <CompA />
  </SuspenseWorkaround>
);

@guochengcheng04
Copy link
Author

guochengcheng04 commented Sep 5, 2022

@urugator Can we use updater.isMounted check whether the target component is loaded? When I changed it like this, the warning disappeared.
In this example, we can't check whether the initialization has been completed through componentDidMount. I guess it may be because of fiber. (when the button is clicked for the first time, warning will be triggered)
image

@guochengcheng04
Copy link
Author

and I checked the implementation of the function component you said. It may not be applicable to class components, because componentDidMount does not take effect

@urugator
Copy link
Collaborator

urugator commented Sep 5, 2022

@guochengcheng04

Can we use updater.isMounted check whether the target component is loaded? When I changed it like this, the warning disappeared.

It's only part of the solution, same as here:

if (trackingData.mounted) {
// We have reached useEffect(), so we're mounted, and can trigger an update
forceUpdate()

It may not be applicable to class components, because componentDidMount does not take effect

No it's exactly what it tries to solve:
The problem is that we create reaction during render, but if the component isn't commited (concurrent mode), no effect or lifecycle method is called, but we still have to dispose the reaction created in render.

@guochengcheng04
Copy link
Author

guochengcheng04 commented Sep 5, 2022

@urugator I mean, like the code in the figure, in the function component scenario, we can assign a value to trackingData. mounted through useEffect, because it is accurate. However, in the scenario of class components, if we use the same strategy to assign the value to trackingData.mounted when componentDidMount, a warning will inevitably appear in the next render. We can't mock the implementation of isMounted according to the existing API, or do you have any other ideas?

@urugator
Copy link
Collaborator

urugator commented Sep 5, 2022

trackingData.mounted when componentDidMount, a warning will inevitably appear in the next render

Why? Once the componentDidMount/useEffect is called, it is safe to call forceUpdate/setState until componenWillUnmount/disposer is called. It's exactly the same.

@guochengcheng04
Copy link
Author

guochengcheng04 commented Sep 5, 2022

https://codesandbox.io/s/mobx6-and-react18-2-gu58ts?file=/CompA.js

@urugator Look at this output. componentDidMount has been executed, but the render triggered by clicking the button will still report an error

@guochengcheng04
Copy link
Author

image

@urugator
Copy link
Collaborator

urugator commented Sep 5, 2022

@guochengcheng04 Yes because componentDidMount is called on different component instance, than the one that throws the error. This is what happens:
React creates component instance (+ reaction), renders for the first time, throws component instance away without calling componentDidMount/WillUnmount (reaction isn't disposed), then it creates second component instance (+ reaction), renders for second time, commits the component, calls componentDidMount.
When the state is mutated, we notify both reactions, but the first one (leaking) calls forceUpdate on uncommited component, therefore the warning.

@guochengcheng04
Copy link
Author

guochengcheng04 commented Sep 5, 2022

@urugator so the idea is still to associate the mock mounted flag with reaction one by one, right?

guochengcheng04 pushed a commit to guochengcheng04/mobx that referenced this issue Sep 5, 2022
@guochengcheng04
Copy link
Author

@urugator I implemented the logic of the class component according to the idea of ​​the function component, and the verification passed, please help to see if there is any problem, expect your approval

https://github.com/mobxjs/mobx/pull/3515/files

guochengcheng04 pushed a commit to guochengcheng04/mobx that referenced this issue Sep 5, 2022
guochengcheng04 pushed a commit to guochengcheng04/mobx that referenced this issue Sep 6, 2022
guochengcheng04 pushed a commit to guochengcheng04/mobx that referenced this issue Sep 6, 2022
@guochengcheng04
Copy link
Author

I have completed the repair and verification of this issue, can any maintainer help me merge and publish it?

@github-actions github-actions bot mentioned this issue Mar 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants