Unable to find node on an unmounted component

[punmechanic]

Bug Report

This report demonstrates an issue in which a component which uses <RefFindNode> will cause a React tree crash because findDOMNode(this) is called after a component has been unmounted.

Steps

Here is a Code Sandbox which demonstrates the issue. You should open this in Firefox as Chrome will not display the precise error due to the error being cross-origin.

https://codesandbox.io/s/v8m974m98l

My understanding of the problem is that if a user unmounts a Semantic component that utilises <RefFindNode /> after its initial mount, Semantic may attempt to access the DOM Node of that element after it is unmounted, causing a full app tree crash. A great way to demonstrate this is by using React.useEffect() and throwing a Promise after initial render.

In the sandbox above, you can see this by throwing a Promise to simulate React Suspense from within a Semantic <Modal />, for which the button is a trigger. The Promise is thrown after the first render due to the usage of useEffect() and useState().

This can be taped over reinforced by enclosing the component which throws (FetchyChildren) in React.Suspense which prevents the thrown Promise from propagating up the chain, such that the thrown Promise does not result in the Button being unmounted.

  const modal = (
    <EditModal>
      <React.Suspense>
        <FetchyChildren />
      </React.Suspense>
    </EditModal>
  );

This does appear to be specific to Promises: Throwing a generic error allows the error to propagate normally through to the usual lifecycles. It's only when throwing a Promise that I've seen so far that causes an app tree crash.

Expected Result

There should be no runtime error related to findDOMNode() or full app tree crash.

Full expected behaviour is unclear, since in this case the <Modal /> is the owner of its open/closed state and will get unmounted. I'd need someone more knowledgeable on the ins and outs of how Suspense works with its app tree to know the intended behaviour, but I'm pretty sure a full app tree crash is not it :)

Actual Result

React App tree goes boom

Version

Semantic UI React 0.68.0
React 16.8.6
React DOM 16.8.6

Testcase

Apologies, I don't have time to fork your codesandbox right now because I have to go to a meeting, but I prepared a sandbox while trying to repro the bug from my own code and am happy to port it over though it should be reasonably understandable. Run the tests in Firefox because Chrome will not display the test error.

Unfortunately, it's not possible to use an assertion like expect(() => act(() => button.click())).toThrow() because there will be a weird error to do with code sandbox thrown.

https://codesandbox.io/s/v8m974m98l

This is related to #3255, which states that wrapping your button with forwardRef will solve the issue - Unfortunately, the problem is within Semantic components and not userland ones. One way I have managed to patch over this is to indeed create a custom button component with forwardRef, but Semantic's Button is still broken.

[welcome]

👋 Thanks for opening your first issue here! If you're reporting a 🐞 bug, please make sure you've completed all the fields in the issue template so we can best help.

We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can.

[layershifter]

Excuse me, you're throwing errors and then wondering why errors happen? ¯_(ツ)_/¯

I understand that you want to catch rejection with Suspense, but why you can't do this inside Modal?

<React.Suspense fallback={<h1>Loading..</h1>}>
  <FetchyChildren />
</React.Suspense>

---

Actually, seems that related to facebook/react#14188 and should be fixed in next release by facebook/react#15312.

[punmechanic]

Excuse me, you're throwing errors and then wondering why errors happen? ¯_(ツ)_/¯

I'm throwing a Promise, not an error, which is expected behaviour of React Suspense, and was wondering why Semantic is attempting to locate a node which has been unmounted.

The way you've phrased your reply there carries some unnecessary snark.

I understand that you want to catch rejection with Suspense, but why you can't do this inside Modal?

I absolutely could do that, the reason I raised this report is because:

1. this only occurs with Semantic components in my project (because they are the only ones using findDOMNode()).
2. a complete app crash while using features of React seems like a bug; if that bug originates from within Semantic it stands to reason that raising a bug on the Semantic bug tracker is a good place to start
3. it's was not immediately clear what the cause of the error was if you're not aware of implementation details of Semantic

thank you for the links to the React issue tracker, it does appear that those tickets track the ultimate source of the issue.

[layershifter]

The way you've phrased your reply there carries some unnecessary snark.

New APIs sometimes confusing me because errors outside Suspense should crash 🤔 Looks like you can misunderstand me, sorry, if I was not polite 🙌

Thank you for detailed responses and a repro case (!) ❤️

it's was not immediately clear what the cause of the error was if you're not aware of implementation details of Semantic

Can't agree there, but it can be caused because I know how it's implemented.

[Jean-PhilippeD]

Hi !

I'm new to react.js anf semantic-ui, and I think I get the same issue on Dropdown component.
I have a main menu with Dropdown inside, and each Dropdown.Item have props as={Link}

I use react-router, and each time I click on item, I get the same error:
Unable to find node on an unmounted component.

I tried several things, if I set my Dropdown components to not update (shouldUpdateComponent return false), it does not fails but that's not what I want because the items should update in some cases..