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

Remove React warning when unmounting a docs page #20731

Closed
tmeasday opened this issue Jan 23, 2023 · 5 comments · Fixed by #21214
Closed

Remove React warning when unmounting a docs page #20731

tmeasday opened this issue Jan 23, 2023 · 5 comments · Fixed by #21214

Comments

@tmeasday
Copy link
Member

tmeasday commented Jan 23, 2023

Describe the bug

Warning: Attempted to synchronously unmount a root while React was already rendering. React cannot finish unmounting the root until the current render has completed, which may lead to a race condition.
@tmeasday tmeasday moved this to Required for GA in Core Team Projects Jan 23, 2023
@tmeasday tmeasday moved this from Required for GA to Required for RC in Core Team Projects Jan 23, 2023
@aryatalathi
Copy link

Can I work on this issue? Waiting for a reply. Thank you.

@ndelangen
Copy link
Member

@aryatalathi yes please, that'd be great!

I can assist, if needed.

@tmeasday
Copy link
Member Author

tmeasday commented Jan 27, 2023

Reading this issue:

In short: Triggers a warning if you unmount a root in a ref callback. Also getting the warning of the state update is caused by a non-discrete event.

So seems intended that you use setTimeout instead? Though it is confusing to me that ref callbacks are considered "during rendering".

I think this the line where we do that (cleanup comes from here in react):

So maybe we should just wrap it in a setTimeout(...,0)? I tried that and the warning went away.

If that's the right approach, then I would want to be sure that we don't accidentally break the teardown/cleanup mechanism, which was added in: #18457

@tmeasday
Copy link
Member Author

@aryatalathi - I ended up doing this: #21214

@github-project-automation github-project-automation bot moved this from Required for GA to Done in Core Team Projects Mar 1, 2023
@shilman
Copy link
Member

shilman commented Mar 1, 2023

Hurrah!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.0.0-beta.58 containing PR #21214 that references this issue. Upgrade today to the @next NPM tag to try it out!

npx sb@next upgrade --prerelease

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

Successfully merging a pull request may close this issue.

4 participants