lukasbach / react-complex-tree

Unopinionated Accessible Tree Component with Multi-Select and Drag-And-Drop
https://rct.lukasbach.com
MIT License
944 stars 74 forks source link

Set state on unmounted component #278

Closed kamilkazmierczakMtab closed 9 months ago

kamilkazmierczakMtab commented 1 year ago

Describe the bug TreeEnvironment in some circumstances can call setState on unmounted component. Error I get 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.

Issue occurs because of set state call inside onMissingItems https://github.com/lukasbach/react-complex-tree/blob/main/packages/core/src/uncontrolledEnvironment/UncontrolledTreeEnvironment.tsx#L202

To Reproduce Hard to reproduce - in my case warning is always thrown because component is rendered inside Layout of react-router-dom

Expected behavior Warning is not shown

kamilkazmierczakMtab commented 1 year ago

I tried to make PR with fix but for some reason I get a lot of issues even when trying to start storybook. I tested fix with copy paste of UncontrolledTreeEnvironment so I can assure you that it works

  1. Additional custom hook (you can write it or use from e.g. here https://usehooks-ts.com/react-hook/use-is-mounted)

    function useIsMounted() {
    const isMounted = useRef(false);
    
    useEffect(() => {
    isMounted.current = true;
    
    return () => {
      isMounted.current = false;
    };
    }, []);
    
    return isMounted;
    }
  2. Inside onMissingItems prop of UncontrolledTreeEnvironment
    if (isMountedRef.current) {
    writeItems(...);
    }

At first it looks like setTimeout is an issue but it's not. getTreeItems is a problem and because of the Promise - solution with useIsMounted hook seems reasonable

lukasbach commented 1 year ago

Hey, thanks for the report! I implemented your suggestion in the latest release, please let me know if that works for you.

kamilkazmierczakMtab commented 1 year ago

Thanks for the fix it did in fact fixed that particular issue but for my use case it introduced another one which is quite specific.

Context:

I am using it as navigation tree so on every select I am calling navigate from react-router-dom inside onSelectItems prop.

Issue:

Cannot update a component (`RouterProvider`) while rendering a different component (`ForwardRef`). To locate the bad setState() call inside `ForwardRef

Naive fix

Fix on my side by wrapping navigate in setTimeout

Potential reason

onSelectItems prop inside UncontrolledTreeEnvironment is doing 2 things First it calls onSelectItems prop (which in my case calls navigation), then setState is called. Probably somehow it could lead to navigation being triggered while rendering tree, which I think should not be a case but maybe it is - hard to say. The thing that is not clear to me is why my fix with checking isMounted inside onMissingItems was solving that issue. What is more surprising - while debugging it the warning that I get is thrown before any code inside onMissingItems is called.

lukasbach commented 9 months ago

Sorry for coming back to you so late with this. I had another look, but can't really find anything obvious that might cause that. If the setTimeout workaround works for you, I'd say that's okay, setTimeout is already used for similar things in the library in cases where it wasn't possible otherwise.

kamilkazmierczakMtab commented 9 months ago

I am reopening this issue because it turns out that setTimeout does not fix my issue. I prepared sandbox that shows that problem - https://codesandbox.io/p/sandbox/react-complex-tree-navigation-issue-v4m56y?file=%2Fsrc%2FApp.tsx After clicking on Child item 2 you will get console warning

image

For some reason my solution here https://github.com/lukasbach/react-complex-tree/issues/278#issuecomment-1648594386 is preventing that error from happening - I have copy of UncontrolledTreeEnvironment on my side with that change - I don't understand why your fix https://github.com/lukasbach/react-complex-tree/commit/fa12f134cc5645e68f54139e4227e8e20816bd25 did not fix that issue. I tried to create PR with my change but for some reason I am getting that error all the time (maybe sth important changed in 2.3.0 or I did sth wrong while trying to build my new version of package)

lukasbach commented 9 months ago

I had another look, and with the repro you provided specifically, I noticed that the implementation of the onSelectItems prop might be a bit unfortunate for your case, as whatever happens within that prop is actually performed in the middle of a setState call, so this might very well be related to that issue.

Can you try your problem again with the release version 2.2.5-alpha.0? There I moved that call out of the setState, maybe that is already enough to fix it.

kamilkazmierczakMtab commented 9 months ago

I tested it and it works perfect 🥇

lukasbach commented 9 months ago

Cool happy to hear! I've properly released the change with 2.3.2.