Skip to content

refactor: share Pages ISR cache runtime#684

Open
southpolesteve wants to merge 1 commit intocodex/pages-api-route-runtimefrom
codex/pages-isr-cache-runtime
Open

refactor: share Pages ISR cache runtime#684
southpolesteve wants to merge 1 commit intocodex/pages-api-route-runtimefrom
codex/pages-isr-cache-runtime

Conversation

@southpolesteve
Copy link
Collaborator

Summary

  • swap the Pages Router server entry over to the shared server/isr-cache.ts runtime instead of embedding a second ISR implementation in the generated template
  • keep the generated entry thin by delegating isrGet, isrSet, triggerBackgroundRegeneration, and isrCacheKey through typed helpers
  • update the generator assertions to check delegation instead of the removed inline ISR implementation details

Verification

  • vp check packages/vinext/src/entries/pages-server-entry.ts tests/entry-templates.test.ts
  • vp test run tests/entry-templates.test.ts tests/isr-cache.test.ts -u
  • vp test run tests/pages-router.test.ts -t "renders pages with getStaticPaths + getStaticProps|returns 404 for paths not in getStaticPaths when fallback is false|keeps ISR cache-fill rerenders isolated from the streamed render state|wraps stale regeneration in a fresh unified request context"
  • vp run vinext#build

Written by Codex.

@southpolesteve
Copy link
Collaborator Author

/bigbonk

Copy link
Contributor

@ask-bonk ask-bonk bot left a comment

Choose a reason for hiding this comment

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

Clean refactor that does exactly what the AGENTS.md guidelines prescribe: move real behavior out of the generated entry into a typed, testable module, keeping the codegen layer as thin delegation glue.

Verified:

  1. getRequestExecutionContext identity -- The generated entry imports from shims/request-context.js (aliased as _getRequestExecutionContext), and server/isr-cache.ts imports the same function from ../shims/request-context.js. Both resolve to the same module. The shared module's getRequestExecutionContext()?.waitUntil(promise) is equivalent to the old inline if (ctx && typeof ctx.waitUntil === "function") ctx.waitUntil(promise) pattern.

  2. tags default handling -- The old inline isrSet used tags || []. The shared isrSet uses tags ?? []. Both produce [] for undefined/null inputs. No behavioral regression.

  3. isrCacheKey buildId injection -- The thin wrapper correctly passes buildId || undefined, matching the old inline behavior where buildId was a top-level const that could be null. The || undefined coercion ensures the shared function's optional parameter semantics work correctly.

  4. pendingRegenerations scope -- The old inline code used a module-scoped new Map(). The shared module uses Symbol.for() on globalThis for cross-environment dedup. This is strictly better for the Pages Router case since it means the dedup map is shared if the module is loaded in multiple Vite environments.

  5. Tests -- Assertions correctly shifted from checking inline implementation details (regex for renderFn() ordering, waitUntil placement, fnv1a64) to checking delegation patterns (import presence, wrapper function shapes, absence of removed code).

One observation (not blocking): the App Router entry still inlines its own ISR helpers with a comment explaining why (RSC environment module-evaluation constraints). A follow-up to share those too would further reduce duplication, but the comment documents the constraint clearly.

@ask-bonk
Copy link
Contributor

ask-bonk bot commented Mar 25, 2026

Review posted and PR approved. Here's a summary of what I verified:

PR #684 swaps the Pages Router server entry from an inline ISR implementation to delegating to the shared server/isr-cache.ts runtime. The generated entry now contains only thin wrapper functions that forward to isrGet, isrSet, isrCacheKey, and triggerBackgroundRegeneration from the shared module.

Key verification points:

  • Both the generated entry and isr-cache.ts resolve getRequestExecutionContext from the same shims/request-context.js module -- waitUntil registration is preserved
  • The tags ?? [] default in the shared isrSet is equivalent to the old tags || []
  • The buildId || undefined coercion in the isrCacheKey wrapper correctly handles the nullable buildId const
  • The shared module's Symbol.for() dedup map is actually an improvement over the old module-scoped Map
  • Tests properly shifted from asserting on inline implementation details to asserting on delegation patterns

github run

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.

1 participant