fix(nitro): add json extension to payload cache items#35043
Conversation
This fixes nuxt#34961 which is caused by the url being used as is to store the cache items. The problem is urls can have nesting (eg: `/mypath`, `/mypath/mysubpath`) But the fs driver does not differentiate between folders and files. This means /mypath become a file with the cache content and when you try to write /mypath/mysubpath it fails because /mypath is a file and not a directory. With this PR: /mypath will store as /mypath.json /mypath/mysubpath will store as /mypath/mysubpath.json There is no conflict issue
|
|
@nuxt/kit
@nuxt/nitro-server
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nitro-server/src/runtime/handlers/renderer.ts (1)
184-186:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Cache key mismatch – line 185 must also use
.jsonextension.Line 185 writes the payload to cache using
ssrContext.urlwithout the.jsonextension, but lines 127–128 read usingurl + '.json'. This mismatch means payloads cached via this path will never be retrieved.Scenario:
- First request to
/mypath/_payload.jsonduring prerender- Line 127 checks cache for
/mypath.json→ miss- Payload is rendered and cached at line 185 as
/mypath(no extension)- Subsequent request to
/mypath/_payload.json- Line 127 checks cache for
/mypath.json→ miss (cache entry is/mypath)🐛 Proposed fix to align cache key with read path
const response = renderPayloadResponse(ssrContext) if (import.meta.prerender) { - await payloadCache!.setItem(ssrContext.url, response) + await payloadCache!.setItem(ssrContext.url + '.json', response) }🤖 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/nitro-server/src/runtime/handlers/renderer.ts` around lines 184 - 186, Cached payloads are written with a key that omits the ".json" extension which doesn't match the read path that looks up url + '.json'; update the write in the import.meta.prerender branch to use the same cache key format (append ".json" to ssrContext.url when calling payloadCache!.setItem) so payloadCache.setItem(ssrContext.url + '.json', response) aligns with the lookup logic used elsewhere (where url + '.json' is read).
🧹 Nitpick comments (1)
packages/nitro-server/src/runtime/handlers/renderer.ts (1)
195-195: ⚡ Quick winVerify cache key normalisation consistency across read and write paths.
The write path normalises the URL (removes trailing slashes except for root) before appending
.json, whilst the read path on line 124 extracts the base URL from the payload request path. Whilst these should produce matching keys in normal cases, consider extracting the normalisation logic into a helper function to ensure consistency and prevent future drift.Example helper:
function normalizePayloadCacheKey(url: string): string { const normalized = url === '/' ? '/' : url.replace(/\/$/, '') return normalized + '.json' }This would make both paths use the same logic and improve maintainability.
Additionally, run the following verification script to check for any other
payloadCacheoperations that might need updating:#!/bin/bash # Description: Find all payloadCache operations to ensure complete .json migration # Search for all payloadCache method calls rg -n -C3 'payloadCache\s*!\s*\.\s*(setItem|getItem|hasItem|removeItem)' --type=ts🤖 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/nitro-server/src/runtime/handlers/renderer.ts` at line 195, The write path for payloadCache uses inline URL normalization which may diverge from the read path; extract the normalization into a single helper (e.g. normalizePayloadCacheKey) that takes ssrContext.url (or the incoming request path) and returns the normalized key with the ".json" suffix, then replace the inline logic in the write site (where renderPayloadResponse is stored via payloadCache!.setItem) and in the read path (the code that calls payloadCache!.getItem / hasItem / removeItem) to call this helper so both use identical logic; also search for all payloadCache usages (setItem/getItem/hasItem/removeItem) and update them to use the helper (you can run the provided ripgrep script to find occurrences).
🤖 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.
Outside diff comments:
In `@packages/nitro-server/src/runtime/handlers/renderer.ts`:
- Around line 184-186: Cached payloads are written with a key that omits the
".json" extension which doesn't match the read path that looks up url + '.json';
update the write in the import.meta.prerender branch to use the same cache key
format (append ".json" to ssrContext.url when calling payloadCache!.setItem) so
payloadCache.setItem(ssrContext.url + '.json', response) aligns with the lookup
logic used elsewhere (where url + '.json' is read).
---
Nitpick comments:
In `@packages/nitro-server/src/runtime/handlers/renderer.ts`:
- Line 195: The write path for payloadCache uses inline URL normalization which
may diverge from the read path; extract the normalization into a single helper
(e.g. normalizePayloadCacheKey) that takes ssrContext.url (or the incoming
request path) and returns the normalized key with the ".json" suffix, then
replace the inline logic in the write site (where renderPayloadResponse is
stored via payloadCache!.setItem) and in the read path (the code that calls
payloadCache!.getItem / hasItem / removeItem) to call this helper so both use
identical logic; also search for all payloadCache usages
(setItem/getItem/hasItem/removeItem) and update them to use the helper (you can
run the provided ripgrep script to find occurrences).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3deea341-087d-46d2-8382-a141f30738d7
📒 Files selected for processing (1)
packages/nitro-server/src/runtime/handlers/renderer.ts
Merging this PR will not alter performance
Comparing Footnotes
|
This fixes #34961 which is caused by the url being used as is to store the cache items. The problem is urls can have nesting (eg:
/mypath,/mypath/mysubpath)🔗 Linked issue
resolves #34961
unjs/unstorage#775
📚 Description
This fixes #34961 which is caused by the url being used as is to store the cache items. The problem is urls can have content at each nesting level (eg:
/mypath,/mypath/mysubpath) but a filesystem cannotSince the fs driver does not differentiate between folders and files. This means
/mypathbecomes a file with the cache content and when you try to write/mypath/mysubpathit fails because/mypathis a file and not a directory.With this PR there is no conflict issue because
/mypathwill store as/mypath.json/mypath/mysubpathwill store as/mypath/mysubpath.jsonThis should be backported to both 3.x and 4.x as this issue is wrecking havock on both versions since this feature was introduced.
I am not sure if the cache would be cleared automatically after a new build, so anyone affected might need to clear the cache directory manually.
Another option would be to use the sha-256 hash of the url as the cache key as is done in the other cache driver. This would avoid the issue of the cache needing to be cleared as well. And will avoid any kind of issues with fs path normalization.