Skip to content

fix: ensure context is consistent on concurrent match loads#1891

Closed
freshgiammi wants to merge 3 commits intoTanStack:mainfrom
freshgiammi:beforeload_context_preload
Closed

fix: ensure context is consistent on concurrent match loads#1891
freshgiammi wants to merge 3 commits intoTanStack:mainfrom
freshgiammi:beforeload_context_preload

Conversation

@freshgiammi
Copy link
Copy Markdown
Contributor

If a match has multiple load routines running (such as navigation happening while preload isn't finished yet), we might start the loader function too early when the context isn't ready for consumption yet.

This happens because execution flow is skipped when a route already in beforeLoad fetching state is re-loaded.

This patch adds beforeLoad tracking to matched routes, so that we can eventually await any pending beforeLoad function and ensure we have up to date context to feed into the loader function.

Fixes #1886

@nx-cloud
Copy link
Copy Markdown
Contributor

nx-cloud Bot commented Jul 4, 2024

☁️ Nx Cloud Report

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

checkLatest()

// Kick off the loader!
loaderPromise = route.options.loader?.(loaderContext)
Copy link
Copy Markdown
Contributor Author

@freshgiammi freshgiammi Jul 5, 2024

Choose a reason for hiding this comment

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

Weirdly enough, doing

matches[index] = match = updateMatch(
  match.id,
  (prev) => ({
    ...prev,
    loaderPromise: route.options.loader?.(loaderContext),
   }),
)

instead of this breaks navigate from working 🤔

@freshgiammi freshgiammi marked this pull request as ready for review July 5, 2024 14:33
schiller-manuel added a commit that referenced this pull request Jul 6, 2024
this currently fails!

AssertionError: expected "spy" to be called 2 times, but got 1 times
 ❯ tests/routeContext.test.tsx:417:18
    415|
    416|     // Expect double call: once from preload, once from navigate
    417|     expect(mock).toHaveBeenCalledTimes(2)
       |                  ^
    418|   })
    419|
@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 beforeload_context_preload 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.

preload: undefined context in loader if navigation happens before beforeLoad is done

2 participants