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]: Can we check shouldRevalidation before we cancel a loader? #12007

Open
CurryYangxx opened this issue Sep 18, 2024 · 4 comments
Open

[Bug]: Can we check shouldRevalidation before we cancel a loader? #12007

CurryYangxx opened this issue Sep 18, 2024 · 4 comments
Labels

Comments

@CurryYangxx
Copy link

CurryYangxx commented Sep 18, 2024

What version of React Router are you using?

6.26.2

Steps to Reproduce

router config: 
{
    path: 'loader',
    shouldRevalidate: () => false,
    loader: () => {
        fetch('/api')
    }
},
{
    path: 'action',
    action: () => {}
}

function Comp () {
    useEffect(() => {
        fetch1.load('/loader');
    }, [])

    useEffect(() => {
        fetch2.submit('/action');
    }, [])

};

When we have code like this. When the component mount, the action submission will cancel the loader. After the action excution, the loader will run again because of revalidation. So we will fetch /api twice.

Expected Behavior

I think you need to check the shouldRevalidate function before cancel a loader, because if the loader don't need to revalidate when this action submit, cancellations are pointless and lead to repeated execution.

Actual Behavior

The loader repeats.

@brophdawg11
Copy link
Contributor

Does this happen if you are using separate fetchers for load and submit? A given fetcher can only have one call in flight at a time, so if you submit from the same fetcher there is no choice but to cancel the in-flight fetcher.load...

@CurryYangxx
Copy link
Author

@brophdawg11 sorry, there is a typo. I am using separate fetchers for load and submit.

@BrianWoolfolk
Copy link

Hi! Is anybody working on this issue? If not, I'd like to give it a try.
In the past days I've been thinking of this one a little bit and while I think this kind of scenario is somewhat uncommon, I totally see the problem at hand. Although, I noticed something:

  1. The problem is inside handleFetcherAction at the start, it calls interruptActiveLoads() (interestingly enough, in some tests I made it seems like some loaders get instantly cut off, and others don't, but their resulting data don't get stored/evaluated and thus need revalidation)
  2. interruptActiveLoads() actually marks the fetchers as 'cancelled', then the action function gets executed, and all 'cancelled' fetchers get forcefully revalidated by getMatchesToLoad afterwards (because of a flag activated by interruptActiveLoads).

The problem is solved if we simply don't interruptActiveLoads and check if a loader is still in process before revalidating it. That way some loaders could even be resolved before the action finishes in an async manner.

The questions I have are about, considering these changes, how do we manage redirections? assume the action quickly redirects somewhere, would the loader be left over? What about multiple-chained loaders? Or error throwing?

This really got my interest; I'll like to send a PR (draft) to address this problem more closely.

@CurryYangxx
Copy link
Author

@brophdawg11 Hi, do you have any feedback?

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

No branches or pull requests

3 participants