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

Bug: False positive warning with nested roots: Attempted to synchronously unmount a root while React was already rendering. #25675

Open
aovchinn opened this issue Nov 14, 2022 · 5 comments
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Type: Discussion

Comments

@aovchinn
Copy link

we get a warning on (legitimate?) use case, if there is a manually added react root inside another parent root
(for example for part of Backbone view that is inserted into parent react component)
on unmount we can see that child root is in commit context, so we cant unmount it

React version: 18.2

Steps To Reproduce

  1. have nested react roots, child is manually added/removed on parent mount/unmount
  2. unmount parent root

I have 2 way bindings for backbone.marionette and react

  • reactToMarionette
  • useMarionetteInReact hook
    when root/parent component gets rendered/destroyed React gives this warning
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.

I am looking for ways to fix this warning

I think ReactChild node is somehow marked as toBeRendered at the app render, even though I would expect that app.root would not know about nested/inserted MView react root

Link to code example: https://codesandbox.io/s/my-test-adapters-forked-tdbgdb

problematic code
import { View } from "backbone.marionette";
import React, { useCallback, useRef, useState } from "react";

import { createRoot } from "react-dom/client";

export function App() {
  // create a reason to render inner component
  const [isVisible, setVisible] = useState(true);
  const toggle = useCallback(() => setVisible((i) => !i), []);

  console.log("render app", isVisible);
  return (
    <>
      <button onClick={toggle}>{`toggle: ${isVisible}`}</button>
      {isVisible ? <SomeComponent /> : null}
    </>
  );
}

// component that children are controlled from outside (by marionette)
const SomeComponent = () => {
  console.log("render SomeComponent");
  const ref = useMarionetteInReact();
  return <div ref={ref} className="stable-react-div"></div>;
};

// hook, for rendering marionette view
const useMarionetteInReact = () => {
  const viewRef = useRef(null);

  const divRef = useCallback((el) => {
    if (el === null) {
      console.log("MView destroy in useCallback", viewRef.current);
      viewRef.current && viewRef.current.destroy();
    } else {
      console.log("created MView");
      const MView = new reactToMarionette({
        className: "reactToMarionette",
        template: false,
        component: <ReactChild />
      });
      viewRef.current = MView;
      MView.render();
      el.appendChild(MView.el);
    }
  }, []);

  return divRef;
};

class reactToMarionette extends View {
  constructor(options) {
    super(options);
    this.component = options.component;
    this.el.textContent = "I am Marionette";

    console.log("create root", this.el);
    this.divEl = document.createElement("div");
    this.divEl.classList.add("portal-root");
    this.root = createRoot(this.divEl);
    this.el.append(this.divEl);
  }

  render() {
    console.log("MView render");
    this.root.render(this.component);
  }

  onBeforeDestroy() {
    console.log("onBeforeDestroy", this.root);
    if (this.root) {
      // setTimeout(() => this.root.unmount());
      this.root.unmount();
    }
  }
}

const ReactChild = () => {
  console.log("render ReactChild");
  return <div> Hello, I am react child </div>;
};

The current behavior

warning is displayed

The expected behavior

no warning ?

@aovchinn aovchinn added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Nov 14, 2022
@gkiely
Copy link

gkiely commented Dec 3, 2022

Related: https://stackoverflow.com/questions/73459382/react-18-async-way-to-unmount-root

@eps1lon
Copy link
Collaborator

eps1lon commented Dec 23, 2022

Smaller repro: https://codesandbox.io/s/react-18-sync-root-unmount-in-ref-callback-sgcy0e

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.

Note that flushSync triggers a similar warning with the explicit nudge to call it in a microtask.

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

@chkhaidzegiorgi
Copy link

do we have any updates on this issue? i am using MFE architecture with module federation, I have same warning when I am calling unmount method while removing the MFE

@markmssd
Copy link

I'm also facing the same issue. It's not obvious to unmount within setTimeout to avoid the error, because that creates race condition with the next mount which intermittently takes a long time to mount (>10s). Is there other alternatives to call root.unmount() without causing this issue? 🙏🏼

@alexvbush
Copy link

any updates on this? experiencing the same issue when I call unmount() in disconnectedCallback of a web component that was added to the parent react root as a ref dom injection.

Legit use case where I have a parent react root add a web component as as ref child and that web component in turn has its own react root, hence the nested react roots.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Type: Discussion
Projects
None yet
Development

No branches or pull requests

6 participants