TanStack / router

πŸ€– Fully typesafe Router for React (and friends) w/ built-in caching, 1st class search-param APIs, client-side cache integration and isomorphic rendering.
https://tanstack.com/router
MIT License
8.18k stars 641 forks source link

Context is undefined after redirect #1531

Closed maksymskuibida closed 5 months ago

maksymskuibida commented 6 months ago

Describe the bug

I use authentication, similar to Authenticated Routes example from documentation. So, when user is on /$lang page, if user is not authorised, user is been redirecting to /$lang/login page. It happens, but useRouteContext returns undefined instead of context. I set context in RouterProvider like this. If I reload the page, issue disappears

<RouterProvider
    router={router}
    context={{ auth, queryClient, showAppAlert }}
/>

I tested on 1.29.2, 1.30.1, 1.31.1, issue happens. I also tested on 1.26.21 and issue does not happen. I didn't test versions between 1.26.21 and 1.29.1

Your Example Website or App

https://admin.7loc.com/en

Steps to Reproduce the Bug or Issue

It happens in our live admin panel(admin panel at all is in development, but some functionality have already been implemented)

Expected behavior

Context, if it set in router provider is always available and not undefined

Screenshots or Videos

https://github.com/TanStack/router/assets/57814533/5d067867-024a-4e8b-9891-ecfd332771f7

Platform

Additional context

No response

maksymskuibida commented 6 months ago

UPDATE.

It happens if I redirect from /$lang to /$lang/login. If I redirect from / to /$lang/login it does not happen. Also attached error, that appears in console

image

https://github.com/TanStack/router/assets/57814533/5566f3c8-1558-4ed2-9889-7413dfbda9a4

SeanCassiere commented 6 months ago

Redirects were broken between 1.28.2 and 1.31.1, but on the side of the Router context, nothing has changed in that area in a LONG time.

We'd need a working reproduction for us to be able to diagnose what specifically in your setup is breaking stuff for you.

Also please confirm this issue you are facing on version 1.31.3 as well.

You can create a reproduction using the Router file-based stackblitz starter.

SeanCassiere commented 6 months ago

I tried to reproduce this and wasn't able to recreate this on my end with version 1.31.3.

Here's my attempt: https://stackblitz.com/edit/github-dxj2gr

mwood23 commented 6 months ago

@SeanCassiere Managed to repro: https://stackblitz.com/edit/github-dxj2gr-btjqhp

mintaka2479 commented 6 months ago

I am having the same issue

https://stackblitz.com/edit/github-dxj2gr-xfuqc1

Case 1

  1. Access /about from the address bar of your browser
  2. Redirect to /foo
  3. Cannot read properties of undefined (reading 'fn') is displayed

Case 2

  1. Added defaultGcTime: 0 to createRouter options
  2. Access Try me! from Home
  3. Cannot read properties of undefined (reading 'fn') is displayed
maksymskuibida commented 6 months ago

@mintaka2479 By any change, do you have any workaround for this issue? Because it brokes login/logout on my website. Now after login and logout I have this issue and user has to reload the page to fix it

budde377 commented 6 months ago

Downgrading to 1.26.21 fixed the issue for us.

maksymskuibida commented 6 months ago

Sometimes it also happens with loaderData

jakst commented 6 months ago

Sometimes it also happens with loaderData

For us, loaderData was fixed in 1.31.6. Context is still sometimes undefined though.

maksymskuibida commented 6 months ago

I am on the latest version and contextI can reproduce in one place every time, but one time in console I see error with both content and loader data on latest version, but I don't know, may be something else caused it, because it was only one time on new version. As I use query, now I almost don't use loaderData, so, I cannot say more about it. But context issue is still happening and cause serious problems. As we still develop our app it is not a critical problem, but I hope it will be fixed soon

ccapndave commented 6 months ago

I am experiencing this too - when redirecting from one route to another in beforeLoad, the context becomes undefined in the target route. It was introduced in v1.31.2.

SeanCassiere commented 6 months ago

Please recheck this.

It's possible that this was inadvertently fixed with #1559.

MAST1999 commented 6 months ago

I'm still facing this problem.

If I redirect the context becomes undefined.

I'm on router version ^1.31.19.

maksymskuibida commented 6 months ago

@SeanCassiere I have just tested on latest version and I am still facing this problem too

mintaka2479 commented 6 months ago

The default options have been fixed. However, if gcTime is 0, the loader’s context becomes undefined after the redirect. There is no problem with beforeLoad.

https://stackblitz.com/edit/github-dxj2gr-xfuqc1

  1. Access Try me! from Home.

console log

1./foo.beforeLoad context {foo: 'bar', fn: Ζ’}
2./foo.beforeLoad context.fn
3./foo.loader context undefined
maksymskuibida commented 6 months ago

I don't have gcTime: 0. I have logout button. When user press on it, I remove auth token and make router.invalidate(), which causes throw redirect to login page in beforeLoad and in this moment context becomes undefined

maksymskuibida commented 6 months ago

I have just tested one more time. Issue happening only if I do throw redirect(). In logic logic I have two options. 1) Url to redirect is specified in query param, in this case I do router.history.push() 2) Url is not specified, in this case after logic I do throw redirect() to home page

So, when I do router.history.push it works. If I do throw redirect(), I am facing undefined context issue, nothing changed for me with the latest version

Issue also happens without router.invalidate() and etc. If I just go to home page anauthorised, what causes throw redirect() in my logic, it brakes with undefined context(video is from production build, in local build it shows like on screenshot)

https://github.com/TanStack/router/assets/57814533/9bb4eacc-e4aa-4a3e-9752-874620ae9182

image
ccapndave commented 6 months ago

I can also confirm that this is not fixed for me.

freshgiammi commented 6 months ago

I've tried taking a stab at it since I've been facing this issue for a bit as well, but I'm hitting a wall. My issue is a bit different from the ones shared in the reproductions (can't share my code, and I haven't managed yet to create a repro), but basically context within loader (not beforeLoad, it's available there) is undefined after a redirect. My routes are like this:

- __root
- __root/_auth
- __root/_auth/_page (Layout shell)
- __root/_auth/_page/ (Index page)
- __root/_auth/_page/unauthorized (Page that redirects the user to Index)

The unauthorized page redirects users from within a beforeLoad to the index page, but that causes the context in the index's loader to be undefined. What I've seen is that once a redirect is executed, the first three routes are cached and retrieved from the Router's store, the and the unauthorized page is swapped with the index page with an empty context (since they both share the same parent tree). Now, apparently all routes are created with no context (here) and get their context from here, however the newly matched route (Index, where the user is being redirected) doesn't reach that code block and consistently halts after executing https://github.com/TanStack/router/blob/main/packages/react-router/src/router.ts#L1563. It looks like the flow just gets interrupted after the beforeLoad is executed, with the Router then failing here as no context can be provided to the loader for the last route matched (because it never gets to merge the context with the matched route).

I'll try making a repro again, but in the meanwhile I hope this can help pinpoint the issue somehow πŸ€·β€β™‚οΈ

maksymskuibida commented 6 months ago

@SeanCassiere By any change, do you know when will this be fixed approximately? It really makes a lot of problems in my app and, unfortunately, I cannot rollback to previous version, because there are other bugs there. Thank you a lot for your work

SeanCassiere commented 6 months ago

I don't have the bandwidth to look into this until probably mid next week.

@tannerlinsley any idea around where this bug would be happening?

bouchardm commented 6 months ago

In the meantime here is a workaround

export const Route = createFileRoute('/a-route/')({
  component: () => {
  const navigate = useNavigate();

  setTimeout(() => {
    navigate({ to: '/another-route' });
  }, 0);

  return null;
}
})

ugly but working on my side

maksymskuibida commented 6 months ago

In the meantime here is a workaround

export const Route = createFileRoute('/a-route/')({
  component: () => {
  const navigate = useNavigate();

  setTimeout(() => {
    navigate({ to: '/another-route' });
  }, 0);

  return null;
}
})

ugly but working on my side

Thank you very much, I will try it

maksymskuibida commented 6 months ago

I tested replacing throw redirect({}) with router.navigate() and now issue does not appear. So, issue appear only with throw redirect(). I event don't need setTimeout, it works without it( I do it in beforeLoad, not in component, so hook is not available )

Patchrik commented 6 months ago

I tested replacing throw redirect({}) with router.navigate() and now issue does not appear. So, issue appear only with throw redirect(). I event don't need setTimeout, it works without it( I do it in beforeLoad, not in component, so hook is not available )

This also worked for me, and I'm on v1.32.2

ccapndave commented 6 months ago

Yup, this workaround does the job :+1:

One thing to be careful of is to include { replace: true } in the options to navigate, otherwise it will put the route onto the stack and probably break the back button.

KirillTregubov commented 6 months ago

I can confirm I am running into the same issue and the culprit seems to be throw redirect().

My reproduction: https://stackblitz.com/edit/github-xhruej?file=src%2Froutes%2Fabout.tsx,src%2Froutes%2Findex.tsx&file=src%2Froutes%2Fabout.tsx

Steps to Reproduce the Bug or Issue

  1. Open the DevTools console.
  2. Notice you are on the About page. If you look in the console, you were redirected on initial load.
  3. Press the "Navigate" button.
  4. Notice you are on the Index page. If you look in the console, you will see that the loader context is undefined.

Expected behavior

  1. Open the DevTools console.
  2. Notice you are on the About page. If you look in the console, you were redirected on initial load.
  3. Press the "Navigate" button.
  4. Notice you are on the Index page. If you look in the console, the loader context is an object containing the queryClient

After navigating to a page that has been redirected from, the loader context should not be undefined. We are suggested by the docs / examples to use loader: ({ context: { queryClient }) => {} when defining loaders, which is not valid if context can be undefined.

freshgiammi commented 6 months ago

I can confirm I am running into the same issue and the culprit seems to be throw redirect().

My reproduction: https://stackblitz.com/edit/github-xhruej?file=src%2Froutes%2Fabout.tsx,src%2Froutes%2Findex.tsx&file=src%2Froutes%2Fabout.tsx

I didn't have time to make a reproduction but this is exactly what I said I'm facing in https://github.com/TanStack/router/issues/1531#issuecomment-2096533191.

An error is thrown here on the offending route (in this case, /),and then the cached match in the router store gets updated (here). However, the route remains in a pending state because the finally statement overrides this by using the scoped match object. On the next iteration, it is then skipped as it's marked with isFetching: true.

Moving the updateMatch call from the finally block to the end of the try block fixes this context issue for me, and so does reverting this commit which was introduced in 1.28.6.

cc @SeanCassiere I don't know what that commit was supposed to fix, so I haven't made a PR reverting it but I'm happy to if needed πŸ‘

SeanCassiere commented 6 months ago

Moving the updateMatch call from the finally block to the end of the try block fixes this context issue for me...

@freshgiammi this seems to do the trick.

SeanCassiere commented 6 months ago

This should now be fixed with the 1.32.11 release.

πŸ“’ Huge shoutout to @freshgiammi for the fix.

ccapndave commented 6 months ago

I'm so sorry to say this, but it still doesn't work for me with 1.32.11 :( This is the bug from hell, eh?

SeanCassiere commented 6 months ago

I'm so sorry to say this, but it still doesn't work for me with 1.32.11 :( This is the bug from hell, eh?

@ccapndave a minimal reproduction will be needed for this to be looked into. You can create a minimal reproduction using the Router file-based stackblitz starter or Router code-based stackblitz starter. Please make sure your reproduction uses a minimum version of 1.32.11.

Edit: I'll keep this open for a few days for anyone else who is still running into a problem matching this issue's description.

KirillTregubov commented 6 months ago

This should now be fixed with the 1.32.11 release.

πŸ“’ Huge shoutout to @freshgiammi for the fix.

1.32.11 fixes my issue/reproduction. Cheers!

callumbooth commented 6 months ago

Unfortunately this isn't working for me in 1.32.11.

I've managed to create a repro in stackblitz: https://stackblitz.com/edit/github-kx4dql-mgkaub?file=src%2Froutes%2Fabout.tsx

I think I've narrowed it down to something related to <StrictMode>.

Repro steps:

  1. Go to the repo link above and open the preview.
  2. Go to the about page and open browser dev tools console
  3. In the browser url, remove the search params, so just /about
  4. See that url bar has changed and the we do not receive any context in the loader.
  5. Reload the page, see that when the url has the search param we don't redirect and do get the context in the loader

Now if you remove StrictMode and retest the issue doesn't happen.

SeanCassiere commented 6 months ago

Now if you remove StrictMode and retest the issue doesn't happen.

That's certainly a weird one. I'm currently having trouble replicating this in a unit test.

I'll have to leave this open until I get more bandwidth to get back to this.

maksymskuibida commented 6 months ago

It is still not fixed for me. I tested on the latest version at issue still persists. I don't sure was it like that before or not,but, I checked in react router devtools and context is undefined in latest route match. In __root and $lang routes context is not undefined, but in $lang.login it is undefined

https://github.com/TanStack/router/assets/57814533/a453da63-c3e6-4de0-8067-cf09a93497cd

maksymskuibida commented 6 months ago

And, again,

image

replacing throw redirect() with router.navigate fixes the issue

freshgiammi commented 6 months ago

Yeah, it looks like this issue might have been fixed only in part. With this reproduction template from this comment (with the packages updated accordingly here) I do see issue 2, while issue 1 has been fixed.

Looks like this time it's this function throwing an error, which halts the flow and skips the context assignment to the matched route.

I won't have time to dig deeper until next week so I'm dropping this here in case anyone wants to look into this issue πŸ‘‹

cpakken commented 5 months ago

I made a reproduction: https://github.com/cpakken/issue-tanstack-router-layout-route-context https://github.com/TanStack/router/issues/1618 Seems like it only affects _layout routes for me on non-initial loads. The context appears in child routes of _layouts and root

SeanCassiere commented 5 months ago

The gcTime problem has been fixed with 1.32.15.

SeanCassiere commented 5 months ago

Now if you remove StrictMode and retest the issue doesn't happen.

@callumbooth this problem has been fixed with the 1.32.16 release.

@maksymskuibida please retest with 1.32.16 using throw redirect() instead of navigate. If this doesn't work for you, we'll need a fresh minimal reproduction of your setup (without any unnecessary setup like auth, components, logging, etc.).

@cpakken Your issue will tracked separately in #1618. This issue is already a bit too crowded.

maksymskuibida commented 5 months ago

@SeanCassiere I've just tested and I can confirm that it is fixed for me now. throw redirect() redirects correctly and there is no undefined context anymore. Thank everyone who was involved for their work!

callumbooth commented 5 months ago

My issue is also resolved, thank you πŸŽ‰

jakst commented 5 months ago

@SeanCassiere Sad to say we're still seeing context being undefined after upgrading to v1.33.4. Although, it has moved from initial load to when preloading other routes on hover. I'll try to create a minimal reproduction and will create a new issue for it.

jakst commented 5 months ago

As promised, here's a new issue with reproduction: https://github.com/TanStack/router/issues/1637

mirague commented 5 months ago

I found another edge case in v1.34.9 that causes context to become undefined if the route that throws the redirect has search params that are being validated through validateSearch, and the validation fails/throws (e.g. with zod).

Here's a complete repro: https://stackblitz.com/edit/tanstack-router-qbczsv?file=src%2Froutes%2Findex.tsx

geoff-harper commented 5 months ago

I found another edge case in v1.34.9 that causes context to become undefined if the route that throws the redirect has search params that are being validated through validateSearch, and the validation fails/throws (e.g. with zod).

Here's a complete repro: https://stackblitz.com/edit/tanstack-router-qbczsv?file=src%2Froutes%2Findex.tsx

@mirague I opened an issue for that here https://github.com/TanStack/router/issues/1656, maybe add your repro there as well?