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

fix(react-router): route-context being available and loaders being called after a redirection #1713

Merged
merged 48 commits into from
Jun 12, 2024

Conversation

SeanCassiere
Copy link
Member

@SeanCassiere SeanCassiere commented Jun 5, 2024

This PR initially was just to add tests for the route-context checks after navigation, but it became a bit more.

  • It checks route context being available after a navigation.
  • It checks route context being available after a redirection.
  • It checks that the destination route loaders are being fired when redirected to.
  • The tests introduced in test(react-router): check the availability of the route context after a redirect on the first load #1710 are also being amended to better simulate a "real-world experience". No more relying on calls to router.navigate(), instead a button is clicked which calls navigate from useNavigate().
  • The tests in packages/react-router/tests/redirects.test.tsx related to route-context have been REMOVED in favor of the ones in packages/react-router/tests/routeContext.test.tsx as they simulate real-world usage.

I've manually tested these changes against the reproductions in the following issues and can confirm that this PR fixes the following.

Context-related issues still remain broken in this though.

Thanks to @freshgiammi pointing out the unnecessary this.load().

Copy link

nx-cloud bot commented Jun 5, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit d65e453. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 4 targets

Sent with 💌 from NxCloud.

@SeanCassiere

This comment was marked as outdated.

@SeanCassiere

This comment was marked as outdated.

@SeanCassiere

This comment was marked as resolved.

@SeanCassiere SeanCassiere force-pushed the test/route-context-on-navigate branch from cb38f0b to 1a03f7f Compare June 6, 2024 10:47
@@ -1548,7 +1548,7 @@ export class Router<
redirect = err
if (!this.isServer) {
this.navigate({ ...err, replace: true, __isRedirect: true })
this.load()
// this.load()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix pointed out by @freshgiammi.

@SeanCassiere SeanCassiere changed the title test(react-router): proper tests for checking route context after a redirect triggered by navigation fix(react-router): route-context being available and loaders being called after a redirection Jun 11, 2024
@SeanCassiere SeanCassiere marked this pull request as ready for review June 11, 2024 22:40
@@ -463,88 +459,3 @@ describe('router.navigate navigation using layout routes resolves correctly', as
expect(router.state.location.pathname).toBe('/g/tkdodo')
})
})

function createContextRouter<TCtx extends Record<string, unknown>>(
Copy link
Member Author

@SeanCassiere SeanCassiere Jun 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed since this is no longer representative of what's actually happening in the browser.
Plus these test-cases are already covered in tests/routeContext.test.tsx.

@krambono
Copy link

Hello,

It seems that this version introduced a bug.

I have a simple router with an index.tsx that redirects to a login page with beforeLoad:

export const Route = createFileRoute('/')({
  beforeLoad: () => {
    throw redirect({ to: '/login' });
  },
});
export const Route = createFileRoute('/login')({
  component: Login,
});

When I refresh my browser on "/", I get a blank page.

It was working in previous versions.

I can provide the entire code later.

@TkDodo
Copy link
Contributor

TkDodo commented Jun 19, 2024

I noticed that writing to the context of one route will now overwrite the context of all routes, so the router context is no longer isolated per route.

this is confirmed working in v1.36.2 but no longer in v1.36.3, and this release just has this fix, so I think we can trace it back here

compare this breadcrumbs example on:

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