fix(link): align visible and intent prefetching#1258
Conversation
Visible App Router links were prefetched as one-shot observer callbacks. That lost the mounted visibility state needed to re-prefetch after cache invalidation or router state changes, and automatic dynamic links could not be upgraded to a full prefetch on hover. The Link shim now keeps mounted prefetch instances, tracks the visible set, exposes an internal ping hook for cache and router commits, and persists the full-prefetch strategy after unstable_dynamicOnHover intent. Regression coverage verifies visible-link cache invalidation and dynamic-on-hover strategy upshift.
commit: |
Pages Router Link prefetch=false should only disable viewport prefetch. The shared gate also blocked hover and touch intent prefetches, which matches App Router but diverges from the Pages Router contract. Split intent eligibility by router mode so App Router keeps the stricter gate while Pages Router intent still prefetches in production. Pages viewport prefetch remains one-shot per mounted link to avoid duplicate document prefetch nodes under the visible-link registry. Regression coverage verifies App Router prefetch=false intent suppression, Pages Router hover and touch intent prefetch, and Pages viewport re-entry dedupe.
NathanDrake2406
changed the title
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(link): align visible and intent prefetching
This is a well-structured PR that brings vinext's Link prefetch lifecycle significantly closer to Next.js behavior. The architecture is clean: a WeakMap tracks per-element instance state, a Set tracks visible App Router links for ping-based rescheduling, and the global __VINEXT_PING_VISIBLE_LINKS__ hook avoids circular dependencies between link.tsx and navigation.ts. I verified all 84 tests pass.
What works well
- Separation of concerns: The
observedLinkPrefetchesWeakMap andvisibleLinkPrefetchesSet cleanly separate DOM-element lifetime from visibility scheduling, matching the Next.jslinks.tsarchitecture. - Router-mode split for intent eligibility: The
canLinkIntentPrefetchfunction correctly gates App Routerprefetch={false}(disables both viewport and intent) while allowing Pages Routerprefetch={false}to still prefetch on hover/touch — matching the divergent Next.js contracts. - Pages one-shot viewport dedup: The
viewportPrefetchedflag prevents duplicate<link rel="prefetch">nodes when Pages links re-enter the viewport or receive pings, which is the right behavior since document prefetch links are persistent DOM nodes. - Cache invalidation re-prefetch flow:
invalidatePrefetchCache()clears both the cache and theprefetchedURL set before pinging, so theprefetched.has(cacheKey)guard insideprefetchUrldoesn't block re-prefetch — this is correct. unstable_dynamicOnHoverupgrade: The hover-time mode upgrade persists to the instance so subsequent pings re-prefetch with"full"— neat design.- Test coverage: The new tests cover the key behavioral contracts (persistent observation, cache invalidation refill, dynamic hover upgrade, router-mode intent split, Pages viewport dedup).
Minor comments below — none blocking.
|
|
||
| function registerVisibleLinkPing(): void { | ||
| if (typeof window === "undefined") return; | ||
| window.__VINEXT_PING_VISIBLE_LINKS__ = pingVisibleLinkPrefetches; |
There was a problem hiding this comment.
Nit: registerVisibleLinkPing is called on every effect setup (every mount of every prefetchable Link). The function is cheap (one property write), but it unconditionally overwrites window.__VINEXT_PING_VISIBLE_LINKS__ on every call. Since it always assigns the same function reference, this is harmless, but you could guard it with a module-level boolean to make the intent clearer and avoid the repeated write:
| window.__VINEXT_PING_VISIBLE_LINKS__ = pingVisibleLinkPrefetches; | |
| function registerVisibleLinkPing(): void { | |
| if (typeof window === "undefined" || window.__VINEXT_PING_VISIBLE_LINKS__) return; | |
| window.__VINEXT_PING_VISIBLE_LINKS__ = pingVisibleLinkPrefetches; | |
| } |
This also documents that the hook is meant to be installed once.
| } | ||
|
|
||
| function pingVisibleLinkPrefetches(): void { | ||
| for (const instance of visibleLinkPrefetches) { |
There was a problem hiding this comment.
The instance.isVisible check inside pingVisibleLinkPrefetches is always true by construction — instances are only in visibleLinkPrefetches when isVisible is true (added on setVisibleLinkPrefetch(_, true), removed on setVisibleLinkPrefetch(_, false)). The check is defensive and doesn't hurt, but worth a comment explaining it's a safety belt rather than a reachable branch, so future readers don't wonder when it could be false.
| return; | ||
| } | ||
| const intentMode = unstable_dynamicOnHover ? "full" : prefetchMode; | ||
| if (unstable_dynamicOnHover && internalRef.current) { |
There was a problem hiding this comment.
Minor observation: The unstable_dynamicOnHover instance upgrade mutates instance.mode to "full", but there's no path that reverts it. If the user navigates away and back (re-mount), a new instance is created so that's fine. But if the localizedHref or prefetchMode changes (effect deps), the cleanup removes the old instance and creates a new one with the original mode — also correct.
Just want to confirm: is there a scenario where the user hovers, the instance gets upgraded to "full", then the component re-renders with unstable_dynamicOnHover={false} without the effect re-running? Since unstable_dynamicOnHover is not in the viewport-prefetch effect's dependency array (only in prefetchOnIntent's useCallback deps), the instance created by the viewport effect would stay upgraded. This seems unlikely in practice (the prop is static), but worth noting.
| } | ||
| const instance = observedLinkPrefetches.get(entry.target); | ||
| if (!instance) continue; | ||
| setVisibleLinkPrefetch(instance, entry.isIntersecting || entry.intersectionRatio > 0); |
There was a problem hiding this comment.
The entry.isIntersecting || entry.intersectionRatio > 0 disjunction matches what Next.js does in links.ts for browser compatibility. Good.
|
|
||
| useLayoutEffect(() => { | ||
| setMountedSlotsHeader(getMountedSlotIdsHeader(stateRef.current.elements)); | ||
| window.__VINEXT_PING_VISIBLE_LINKS__?.(); |
There was a problem hiding this comment.
This is the right place to ping — after setMountedSlotsHeader updates the slot context, visible links will re-prefetch with the correct mounted-slots header. The ping is conditional on treeState.elements changing (the effect dep), which is when slot bindings can change. Clean.
| routerMode: LinkPrefetchRouterMode; | ||
| }): boolean { | ||
| if (input.nodeEnv !== "production" || input.isDangerous) return false; | ||
| return input.routerMode === "pages" || input.prefetch !== false; |
There was a problem hiding this comment.
Clear and correct: Pages Router intent is always allowed (dangerous/dev already filtered), App Router intent respects prefetch !== false. The short-circuit reads naturally.
| } | ||
| prefetched.clear(); | ||
| if (!isServer) { | ||
| window.__VINEXT_PING_VISIBLE_LINKS__?.(); |
There was a problem hiding this comment.
The ordering here is important and correct: prefetched.clear() runs before pingVisibleLinkPrefetches(), so the dedup guard inside prefetchUrl won't block re-prefetch. Good.
|
Review submitted. Here's a summary: Approved with minor inline comments. The PR correctly aligns vinext's Link prefetch behavior with Next.js by:
All 84 tests pass. The inline comments are minor suggestions (guard |
2 participants
Overview
packages/vinext/src/shims/link.tsx,packages/vinext/src/shims/link-prefetch.ts,packages/vinext/src/shims/navigation.ts,tests/link-navigation.test.tsprefetch={false}still prefetches on hover/touch intent.Why
Correct Link prefetching depends on two separate contracts. App Router prefetching needs mounted Link state to survive beyond the first viewport hit so visible links can be rescheduled when cache or router inputs change. Pages Router has a different
prefetch={false}contract: it disables viewport prefetch, but hover and touch intent still prefetch.invalidatePrefetchCache()pings visible App Router links after clearing the cache.prefetch={false}disables both viewport and hover/touch prefetch.prefetch={false}disables viewport prefetch only.unstable_dynamicOnHover, upgrade strategy.What changed
unstable_dynamicOnHoverupgrades hover to full prefetch and persists that strategy for later pings.prefetch={false}viewportprefetch={false}hover/touchprefetchtype.Maintainer review path
packages/vinext/src/shims/link.tsx: registry shape, router-mode detection, visibility transitions, intent strategy upshift, Pages one-shot viewport behavior, cleanup.packages/vinext/src/shims/link-prefetch.ts: trigger/router-mode eligibility split.packages/vinext/src/shims/navigation.ts: cache invalidation and committed URL/router-state ping points.packages/vinext/src/server/app-browser-entry.ts: mounted slot header update before visible-link ping.tests/link-navigation.test.ts: regression coverage for persistent observation, invalidation refill, dynamic hover upgrade, App Routerprefetch={false}intent suppression, Pages Routerprefetch={false}hover/touch intent, and Pages viewport re-entry dedupe.Validation
vp test run tests/link-navigation.test.ts tests/link.test.ts: 84 tests passed.vp check: formatting, lint, and type checks passed.vp check --fixandknip --no-progresssuccessfully.Risk / compatibility
next/linkshim typing with supported runtime values.unstable_dynamicOnHoverremains explicitly unstable.prefetch={false}. Pages viewport prefetch stays one-shot to avoid duplicate<link rel="prefetch">nodes under the persistent observer.Non-goals
References
prefetch={false}intent gateprefetchEnabledto viewport prefetch.prefetchEnabled.