fix(view): implement better context management for loaders - #2757
Closed
Truevoly wants to merge 2 commits into
Closed
fix(view): implement better context management for loaders#2757Truevoly wants to merge 2 commits into
Truevoly wants to merge 2 commits into
Conversation
✅ Deploy Preview for vue-router canceled.
|
Truevoly
marked this pull request as draft
July 20, 2026 13:23
Contributor
📝 WalkthroughWalkthroughChangesNested loader context handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Component
participant useDataLoader
participant load
participant ChildEntries
Component->>useDataLoader: request nested loader
useDataLoader->>load: start or refetch loader
load-->>useDataLoader: settle promise
useDataLoader->>Component: restore rendering context
Component->>useDataLoader: trigger reload
useDataLoader->>ChildEntries: invalidate child targets
ChildEntries->>load: refetch child loaders
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/router/src/experimental/data-loaders/defineLoader.ts (1)
412-425: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the microtask-ordering assumption behind this fix.
This relies on
promise.then(onFinally, onFinally)being attached topromisewhilepromiseis settling (inside its own.finally()), so it runs after any handler already attached earlier (e.g. the caller'sawait), letting the resumed caller code see the restored parent context before it's cleared to[]. This is a correct use of promise-handler attachment ordering, but it's non-obvious and easy to break with a seemingly innocuous refactor (e.g. wrapping/awaitingpromisebefore this line, or returning a promise from the.finally()callback). A short comment explaining why the reset must be scheduled viapromise.then(...)rather than done synchronously would help future maintainers avoid regressing this.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/router/src/experimental/data-loaders/defineLoader.ts` around lines 412 - 425, Add a concise comment in the `.finally()` callback explaining that `promise.then(onFinally, onFinally)` must be attached to the settling promise to schedule the context reset after earlier handlers, including the caller’s await, while avoiding unhandled rejections. State that the reset must remain asynchronous rather than calling `setCurrentContext([])` synchronously, and preserve the existing promise structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/router/src/experimental/data-loaders/defineLoader.ts`:
- Around line 387-398: Update the reload handler to recursively invalidate every
descendant of the current loader, not only the direct entries in entry.children.
Traverse each childEntry and clear its to value, then continue through its
children before invoking load(to, router), so nested loaders such as A → B → C
are fully refetched.
---
Nitpick comments:
In `@packages/router/src/experimental/data-loaders/defineLoader.ts`:
- Around line 412-425: Add a concise comment in the `.finally()` callback
explaining that `promise.then(onFinally, onFinally)` must be attached to the
settling promise to schedule the context reset after earlier handlers, including
the caller’s await, while avoiding unhandled rejections. State that the reset
must remain asynchronous rather than calling `setCurrentContext([])`
synchronously, and preserve the existing promise structure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 13d775b9-5c62-460a-9709-0c17bd0da2fa
📒 Files selected for processing (2)
packages/router/src/experimental/data-loaders/defineLoader.tspackages/router/src/tests/data-loaders/tester.ts
Comment on lines
387
to
+398
| reload: (to: RouteLocationNormalizedLoaded = router.currentRoute.value) => | ||
| router[APP_KEY] | ||
| .runWithContext(() => load(to, router)) | ||
| .runWithContext(() => { | ||
| const entry = entries.get(loader)! | ||
| entry.children.forEach(childEntry => { | ||
| // for child loaders we usually want to avoid refetching, | ||
| // that's why we're checking that requested and loaded routes are the same | ||
| // when reloading, we want to refetch though, and so we make those routes different | ||
| childEntry.to = null | ||
| }) | ||
| return load(to, router) | ||
| }) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether any test exercises 3+ level nested loader reload
rg -n "children" packages/router/src/experimental/data-loaders/defineLoader.ts
rg -n "reload" packages/router/src/tests/data-loaders/tester.ts | head -50Repository: vuejs/router
Length of output: 1845
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the reload implementation and surrounding child-entry logic.
sed -n '300,420p' packages/router/src/experimental/data-loaders/defineLoader.ts
echo '--- tester reload-related tests ---'
sed -n '1540,2035p' packages/router/src/tests/data-loaders/tester.tsRepository: vuejs/router
Length of output: 19975
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all places that mutate or traverse loader children, and inspect the load path.
rg -n "children\.|parentEntry\.children|entry\.children|commit\(to\)|load\(" packages/router/src/experimental/data-loaders/defineLoader.ts
echo '--- load/child relationship slice ---'
sed -n '1,260p' packages/router/src/experimental/data-loaders/defineLoader.ts
echo '--- middle slice around nested-loader association ---'
sed -n '260,420p' packages/router/src/experimental/data-loaders/defineLoader.tsRepository: vuejs/router
Length of output: 15963
🏁 Script executed:
python3 - <<'PY'
# Model the relevant parts of defineLoader.ts:
# - each entry has to, pendingTo, children
# - reload() sets only direct children.to = None
# - useDataLoader() reloads a nested loader only when parentEntry exists and entry.to != route
# - commit() only propagates to current children, but does not itself trigger reloads
class Entry:
def __init__(self, name):
self.name = name
self.to = None
self.pendingTo = None
self.children = []
self.loaded = 0
def commit(self, to):
if self.pendingTo == to:
self.to = to
self.pendingTo = None
for c in self.children:
c.commit(to)
def load(entry, to):
if entry.pendingTo == to:
return
entry.pendingTo = to
entry.loaded += 1
def use_loader(entry, to, parent=None):
if (entry.pendingTo is None) or (parent is not None and entry.to != to):
load(entry, to)
if parent is not None and entry not in parent.children:
parent.children.append(entry)
def reload(entry, to):
for c in entry.children:
c.to = None
load(entry, to)
entry.commit(to)
# Initial chain A -> B -> C at route R1
A, B, C = Entry("A"), Entry("B"), Entry("C")
use_loader(A, "R1")
A.commit("R1")
use_loader(B, "R1", A)
B.commit("R1")
use_loader(C, "R1", B)
C.commit("R1")
# Reload A on the same route
reload(A, "R1")
print("A loaded:", A.loaded)
print("B loaded:", B.loaded)
print("C loaded:", C.loaded)
print("A.to/B.to/C.to:", A.to, B.to, C.to)
print("A.pending/B.pending/C.pending:", A.pendingTo, B.pendingTo, C.pendingTo)
PYRepository: vuejs/router
Length of output: 259
Reload only clears direct children, so deeper descendants can stay stale. In a chain like A → B → C, reloading A resets B’s to, but C keeps its previous route and is reused when B reruns. That mixes fresh parent data with stale nested data; invalidate descendants recursively if 3+ levels are expected.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/router/src/experimental/data-loaders/defineLoader.ts` around lines
387 - 398, Update the reload handler to recursively invalidate every descendant
of the current loader, not only the direct entries in entry.children. Traverse
each childEntry and clear its to value, then continue through its children
before invoking load(to, router), so nested loaders such as A → B → C are fully
refetched.
4 tasks
…text when exiting useDataLoader close vuejs#2684
posva
added a commit
that referenced
this pull request
Jul 23, 2026
Ports the reload regression test from #2757 and adds coverage for reloading deeply nested loaders. Runs for both the basic and the Pinia Colada loaders. Co-authored-by: Ilia Bolotov <Ilia_bolotov@epam.com>
Member
|
Thanks! See #2727 (comment) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
close #2684
This PR is in draft, feel free to provide comments, suggestions, critique
This is my approach to resolve the issue with nested loader's context
I have found another way to reproduce the issue with context and that is by doing reload from component
Since provided solution by posva only updated
useDataLoaderand notloadfunction (which is used when doing reload), I was worried that fix will not work forreloadTake a look at this stackblitz.
There are 4 pages, each page has two loaders that do not depend on each other, and a subpage, that has a loader that depends on those two. Difference between pages is how long it takes for each loader to resolve with a value.
Open console and navigate between pages, click reload after reloading has finished. While page will not go into infinite loop, it still logs a warning about incorrect context.
This stackblitz contains vue-router built from this branch. Open console and check logs, navigate between pages, reload, try and break it (and I mean it, I want this to be bulletproof)
I've started investigating and trying to follow the context during the different phases of execution. As it turns out, since context is only relevant for nested loaders, we can keep it empty for the most time, and only update on demand.
If our loader has nested loaders, it will call
useDataLoader. When nested loader is finished, we reachfinally. Here, we don't know if there are other nested loaders after us, so in case there are, we prepare context for them. We do it only if we're in a nested loader (parentEntryis not undefined). Now, context is restored back to original loader's context.After
finallyfinishes, promise is resolved, and two jobs are put into queue original loader and restoration of context back to empty. After doing some sync work, original loader can potentially call other nested loader, which will receive proper context. After that task is done, context is resolved. Any other call will receive empty contextDuring testing I've spotted a problem with this solution: if you have two nested loaders and second loader resolves before first, when dependent loader reaches second loader it triggers it again. So I've changed the condition on when the loader should be invoked. But now, when doing reload, nested loaders are never triggered. So I force it by resetting their
toproperty, so the condition matches and loader is reloaded. I myself am not a fan of this solution, I welcome any suggestion on how to make it betterSummary by CodeRabbit
Bug Fixes
Tests