remix-run / react-router

Declarative routing for React
https://reactrouter.com
MIT License
52.41k stars 10.18k forks source link

[Bug]: fetcher.load revalidate when navigation submit without running revalidate function #11723

Open CurryYangxx opened 1 week ago

CurryYangxx commented 1 week ago

What version of React Router are you using?

6.23.0

Steps to Reproduce

use this repo branch: https://github.com/Kong/insomnia/tree/perf/modify-initial-entry

we have a loader fetch here: https://github.com/Kong/insomnia/blob/f364d717df2e37b542443586fe09656b46579350/packages/insomnia/src/ui/routes/project.tsx#L614

When trigger navigate in this line first time: https://github.com/Kong/insomnia/blob/f364d717df2e37b542443586fe09656b46579350/packages/insomnia/src/ui/routes/project.tsx#L1051

The permission loader revalidates without running revalidate function in the config.

I dig into the source code.I found it go into this if condition.The loader was canceled because of action submit in this Component.But I found finally the loader is fulfilled and can get data from fetcher. I think maybe it should be deleted from the cancelledFetcherLoads array.Is there any race condition issue?

else if (cancelledFetcherLoads.includes(key)) {
      // Always revalidate if the fetcher was cancelled
      shouldRevalidate = true;
    }

https://github.com/remix-run/react-router/blob/77fca6e9f975b3b9732429085cacaf008d4a9845/packages/router/router.ts#L4397

Expected Behavior

The loader should determine if revalidate through the revalidate function.

Actual Behavior

The loader revalidate when navigation.

brophdawg11 commented 2 days ago

Could you put together a minimal reproduction of the issue you're seeing so we can be sure we understand the problematic behavior?

CurryYangxx commented 2 days ago

Could you put together a minimal reproduction of the issue you're seeing so we can be sure we understand the problematic behavior?

@brophdawg11 This a sandbox for a demo to reproduce. https://codesandbox.io/p/devbox/react-router-test-forked-6nmdty?file=%2Fsrc%2Froute%2Fchild.tsx%3A11%2C129

const loadFetcher = useFetcher();
fetcher.load('/loader')

const submitFetcherA = useFetcher();
fetcher.submit('/actionA');

const submitFetcherB = useFetcher();
fetcher.submit('/actionB');

I have one fetch.load and two fetcher.submit in my component, when the first action submit, the loadFetcher will be aborted in interruptActiveLoads and will add the fetcher key into cancelledFetcherLoads. Then the second action submit, the loadfetcher will be revalidated and finally executed successfully, but the cancelledFetcherLoads dosen't delete the key.

So when I trigger navigate first time, because of this line, the fetcher will be revalidate even if I return false in shouldRevalidate function.And finally the cancelledFetcherLoads will be empty in completeNavigation function. https://github.com/remix-run/react-router/blob/77fca6e9f975b3b9732429085cacaf008d4a9845/packages/router/router.ts#L4397

So I think we need to delete the key from cancelledFetcherLoads after loaderFetcher revalidated success.

Also when I quickly click the navigate1 and navigate2 button in the demo, sometimes there is an error in the console.

image
brophdawg11 commented 1 day ago

Hm, that link is broken for me:

Screenshot 2024-07-02 at 2 38 26 PM

CurryYangxx commented 1 day ago

@brophdawg11 Sorry, now I set the devbox to public access: https://codesandbox.io/p/devbox/react-router-test-forked-6nmdty you can look into the child.tsx