router icon indicating copy to clipboard operation
router copied to clipboard

AbortController abort not handled properly for data loader

Open orangeswim opened this issue 1 year ago • 9 comments

Describe the bug

EDIT: Originally titled Strict Mode not handled properly. Changed because bug happens as well when out of strict mode. See my latest response.

See stackblitz link on changes from data example.

Instructions on usage of abortController, https://tanstack.com/router/v1/docs/guide/data-loading#using-the-abort-signal Per react documentation, React.StrictMode is supposed to make sure your components handle unmount and remount properly.

Visit the stackbllitz link, navigate to an individual post. Then hit refresh on that page (in the frame) so that StrictMode will mount the route twice.

Actual behavior, route stays in aborted mode. In a different copy of the code I printed out via console.log, signal.aborted, which is a boolean. It shows it is aborted on both mounts.

Your Example Website or App

https://stackblitz.com/edit/tanstack-router-yexyqa?file=src%2Fmain.tsx

Steps to Reproduce the Bug or Issue

  1. Go to stackblitz
  2. Navigate to Posts then click a post so you are on a path like https://qgnimrlxlgithub-bmvg--3001--d3416dfd.local.webcontainer.io/posts/1
  3. Refresh the frame inside stackblitz to trigger StrictMode
  4. Component shows an error

Expected behavior

Expected behavior, even if the route has an error, because we are unmounting, this error should be discarded. The newly mounted route (second time) should behave normally.

Screenshots or Videos

image

Platform

  • OS: [macOS 14.1.2 (23B2091)]
  • Browser: [Chrome Version 119.0.6045.199 (Official Build) (arm64), Safari Version 17.1.2 (19616.2.9.11.12)]

Additional context

Would have to turn off strict mode in development which is not ideal.

orangeswim avatar Dec 11 '23 10:12 orangeswim

It looks like a new abortController isn't created even if we try to revisit the route.

  1. Change loader to have a 1 second delay.
  2. Enter into aborted state for /posts/1
  3. Routes entering /posts/1 will always get an abortController that is already aborted.
  4. Switch to /posts/2 and observe abortController is false. Switch back before page loads and /posts/1 is still aborted.
  5. Switch to /posts/3 again and wait until it complets.
  6. This time switching to /posts/1 provides a new abortController

Looking at more console logs, the match is stored in state.matches

orangeswim avatar Dec 11 '23 22:12 orangeswim

We can also enter the bad state with a different method. Remove strict mode

  1. Change loader to have a 1 second delay.
  2. Visit a route. Before it finishes loading, click a link to it again so the page will try to load
  3. This sends an abort signal to the prior request
  4. Aborted error makes it out of the component
  5. Subsequent navigation to the same match will still have the same abortcontroller true

orangeswim avatar Dec 11 '23 22:12 orangeswim

Issue above exists for release beta 254. Pull request is a fork of beta 254

orangeswim avatar Dec 12 '23 02:12 orangeswim

This should be fixed in latest 🎉

tannerlinsley avatar Dec 12 '23 03:12 tannerlinsley

Please reopen this issue. Tested with the latest beta#260 and the error persists.

When an abort happens, it does not trigger handleErrorAndRedirect function. That is where the fix was implemented.

The load function goes all the way through.

router.ts	load	
router.ts	cancelMatches	
router.ts	loadMatches	
router.ts	load	
router.ts	cancelMatches	
router.ts	loadMatches	
Event 1	Error received on react component error boundary, caught by the fetch promise	
Event 2	Error received on react component error boundary, caught by the fetch promise	
RouterProvider.tsx	onResolved emitted

When the component is loaded a second time, state.matches has the current route with the aborted signal. The same route is returned back to the user.

orangeswim avatar Dec 12 '23 07:12 orangeswim

Also thank you for the fast responses! Appreciate your time and effort.

orangeswim avatar Dec 12 '23 07:12 orangeswim

You bet! Happy to let you PR this one again and make sure it passes your tests. I didn't know you were working on a fix for the last one, so I can hold off if you want to tackle it. :)

tannerlinsley avatar Dec 12 '23 16:12 tannerlinsley

Okay so it passes the original issue with React.StrictMode unmounting and remounting. It also passes when you're at page A , then to B, then to A without letting B finish.

However there is a different state where you're at page A-original, then to A-first, then to A-second without letting the A-first finish. A-x is all the same route. A-first is pending, A-second cancels the A-first pending route. A-second reuses the cancelled A-first. A-first match makes it to the page and throws the match.error in MatchInner().

 // Waste not, want not. If we already have a match for this route,
      // reuse it. This is important for layout routes, which might stick
      // around between navigation actions that only change leaf routes.
      const existingMatch = getRouteMatch(this.state, matchId)

https://github.com/orangeswim/tanstackrouter/blob/ee51c0b452d73f9c138eccb7c11ecb3d35449e9a/packages/react-router/src/router.ts#L638C6-L641C63

This gets the existing cancelled pendingRoutes.

I'm not sure what a good solution would be right now. Maybe it would be better to put this in a new issue altogether. Scenario: User double clicks the same link on a page that loads data.

Thoughts?

Using https://stackblitz.com/edit/tanstack-router-yexyqa?file=src%2Fmain.tsx at beta#265 go to /posts/1. Then click on the link for /posts/2 right after the other.

I'll leave this alone for now and keep working on my app.

orangeswim avatar Dec 13 '23 09:12 orangeswim

I'm not completely sure what I'm looking for still, but can you try the latest for this?

tannerlinsley avatar Dec 20 '23 02:12 tannerlinsley