Skip to content

Correctly load matched routes when navigated before preload is finished#1876

Closed
freshgiammi wants to merge 2 commits intoTanStack:mainfrom
freshgiammi:preload_issues
Closed

Correctly load matched routes when navigated before preload is finished#1876
freshgiammi wants to merge 2 commits intoTanStack:mainfrom
freshgiammi:preload_issues

Conversation

@freshgiammi
Copy link
Copy Markdown
Contributor

Ref: #1534
Ref: #1638

@nx-cloud
Copy link
Copy Markdown
Contributor

nx-cloud Bot commented Jul 2, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit bb19c1b. 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.

@freshgiammi
Copy link
Copy Markdown
Contributor Author

There's still an issue here when with context when it gets merged with beforeLoad.

export const Route = createFileRoute('/about')({
  beforeLoad: async () => {   
    await new Promise((resolve) => setTimeout(resolve, 5000));
    return { someData: 'hello' };
  },
   loader: async ({context}) => {
    await new Promise((resolve) => setTimeout(resolve, 2000));
    console.log(context.someData)
  },
})

Supposing /about is the lazy route that takes a long time to resolve, if beforeLoad isn't finished before the navigation event happens, the loader gets invoked with an undefined context, breaking the route.

This possibly happens because execution flow is skipped when a route already in beforeLoad fetching state is re-loaded (because the loader gets invoked regardless of the beforeLoad state, for which we have no pending information on reload)

@schiller-manuel
Copy link
Copy Markdown
Contributor

are you able to reproduce this in a test somehow? this would be awesom.

Comment thread packages/react-router/src/link.tsx Outdated
options: UseLinkPropsOptions<TRouter, TFrom, TTo, TMaskFrom, TMaskTo>,
): React.AnchorHTMLAttributes<HTMLAnchorElement> {
const router = useRouter()
const isClicking = React.useRef(false)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why do we need to track if a link was clicked?
what happens if we have two link instances pointing to the same target, this then won't work, right?

i think the solution must be independent from click tracking etc, and also must work when programatically preloading / navigating without clicking on a link

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this most likely needs to be fixed inside of router.ts

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is a different issue than the references (so we could have it in a separate PR), but essentially onFocus is also invoked when a button is clicked, which causes the route to get preloaded twice (onMouseEnter + onFocus).

When focusing a link via keyboard navigation, we obviously skip onMouseEnter so it gets correctly preloaded once.
Given that there's no way to distinguish between mouse and keyboard origin for focus events, we can "trap" it by skipping the preload call when a focus event is detected between onMouseDown and onMouseUp: this makes preload only happen once and ensures that the behaviour is the same for both mouse and keyboard navigation.

Copy link
Copy Markdown
Contributor

@schiller-manuel schiller-manuel Jul 2, 2024

Choose a reason for hiding this comment

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

ok please put this in a separate PR then

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@freshgiammi
Copy link
Copy Markdown
Contributor Author

are you able to reproduce this in a test somehow? this would be awesom.

The issue I'm still facing in #1876 (comment) should be easily reproducible.

A test case for this preload issue when lazy components haven't finished fetching yet might be more complicated, but I haven't worked on either yet. Still, it could be something close to what I've done in the reproduction template

@freshgiammi
Copy link
Copy Markdown
Contributor Author

Split #1876 (comment) into a separate issue: #1886

@freshgiammi freshgiammi marked this pull request as ready for review July 3, 2024 11:01
schiller-manuel added a commit that referenced this pull request Jul 6, 2024
@schiller-manuel
Copy link
Copy Markdown
Contributor

closing in favor of #1907
I copied the test out of this PR, so thanks for this!

@freshgiammi freshgiammi deleted the preload_issues branch July 7, 2024 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants