fix(nuxt): decode URI components in cache driver methods #30973
Conversation
|
|
WalkthroughThe changes update the key processing in the cache-driver module by modifying the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/nuxt/src/core/runtime/nitro/cache-driver.js (1)
21-26: Consider adding error handling for decodeURIComponent.Whilst the implementation is correct,
decodeURIComponentcan throw URIError for malformed input.Consider wrapping the decode operation in a try-catch:
async setItem (key, value, opts) { + let decodedKey + try { + decodedKey = decodeURIComponent(key) + } catch (err) { + decodedKey = key + } await Promise.all([ - fs.setItem?.(normalizeFsKey(decodeURIComponent(key)), value, opts), + fs.setItem?.(normalizeFsKey(decodedKey), value, opts), lru.setItem?.(key, value, opts), ]) },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/nuxt/src/core/runtime/nitro/cache-driver.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: test-fixtures (ubuntu-latest, built, webpack, default, manifest-on, json, 18)
- GitHub Check: test-fixtures (ubuntu-latest, built, webpack, async, manifest-on, json, 18)
- GitHub Check: test-fixtures (ubuntu-latest, built, rspack, default, manifest-on, json, 18)
- GitHub Check: test-fixtures (ubuntu-latest, built, rspack, async, manifest-on, json, 18)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, default, manifest-off, json, 18)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, default, manifest-on, json, 18)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, async, manifest-off, json, 18)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, async, manifest-on, js, 18)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, async, manifest-on, json, 18)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, default, manifest-off, json, 18)
- GitHub Check: typecheck (windows-latest, node)
- GitHub Check: typecheck (windows-latest, bundler)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, async, manifest-off, json, 18)
- GitHub Check: typecheck (ubuntu-latest, node)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, async, manifest-on, js, 18)
- GitHub Check: typecheck (ubuntu-latest, bundler)
- GitHub Check: release-pr
- GitHub Check: test-benchmark
- GitHub Check: test-size
- GitHub Check: code
🔇 Additional comments (1)
packages/nuxt/src/core/runtime/nitro/cache-driver.js (1)
23-23: LGTM! The changes effectively address the multilingual slug issue.The addition of
decodeURIComponentbefore normalising the file system keys ensures proper handling of encoded characters, preventing potential "ENAMETOOLONG" errors with multilingual slugs. The implementation maintains consistency across all file system operations whilst preserving the original keys in the LRU cache.Let's verify the changes handle various edge cases correctly:
Also applies to: 28-28, 31-31
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Test the changes with various multilingual and special characters # Test 1: Check for any existing error handling for decodeURIComponent rg -A 2 "try.*decodeURIComponent" . # Test 2: Look for similar patterns in other parts of the codebase rg -A 2 "normalizeFsKey.*decodeURIComponent" .Length of output: 978
Verification Complete – Cache-Driver Methods Appropriately Handle Encoded Keys
The grep output confirms that the
decodeURIComponentis correctly applied in all relevant methods (setItem,hasItem, andgetItem). This ensures that the file system keys are properly decoded before normalisation, effectively addressing issues with multilingual slugs.
- setItem (Line 23): Uses
normalizeFsKey(decodeURIComponent(key))for file system storage.- hasItem (Line 28): Checks the existence using
normalizeFsKey(decodeURIComponent(key)).- getItem (Line 31): Retrieves the item via
normalizeFsKey(decodeURIComponent(key)).All changes are consistent and align with the PR objectives.
@nuxt/kit
@nuxt/rspack-builder
nuxt
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
CodSpeed Performance ReportMerging #30973 will not alter performanceComparing Summary
|
| * @param {string} item | ||
| */ | ||
| const normalizeFsKey = item => item.replaceAll(':', '_') | ||
| const normalizeFsKey = item => decodeURIComponent(item.replaceAll(':', '_')) |
There was a problem hiding this comment.
my only worry is whether windows/mac/linux filesystems will all support non-ASCII characters. (maybe, if not, we need an additional sanitisation step)
There was a problem hiding this comment.
That's a really good point, thanks for the feedback!
I’ve given this a lot of thought, and here’s my reasoning:
-
Initial Approach:
I initially considered that users might supply virtually any string for the article slug. Since we can’t predict which characters they might use, I thought about normalizing the input by applying a hashing function (e.g., SHA-256 or a lighter alternative). This would ensure that the output is consistent and only contains allowed characters. -
Reevaluation:
After looking at the bigger picture, I realized that the files created by thecache-driverduring thenuxt generatecommand are temporary, they’re only used for generation. What really matters is that in the final output, under the.outputfolder, users see a sub-folder with the original name they expect.
For example:
-
During Generation:
<rootDir>/.nuxt/cache/nitro/prerender
-
Final Output:
<rootDir>/.output/public
Let me know if you see any issues with this approach or if there’s another angle I might have missed.
There was a problem hiding this comment.
what approach are you suggesting?
There was a problem hiding this comment.
I suggest we stick with the decodeURIComponent approach to avoid any performance overhead from hashing. Since the temporary cache file and the final output folder share the same name, any system that doesn't support non-ASCII characters will fail to create both. If I implement a solution for the temporary cache file, then it will fail when creating the folder.
🔗 Linked issue
Resolves #27722
📚 Description
When using Nuxt version 3.11 or above, pre-rendering pages with multilingual slugs (including non-ASCII characters) causes filenames to be encoded to US-ASCII. This encoding significantly increases the filename length, potentially exceeding the file system's 255-character limit, and ultimately triggers an "ENAMETOOLONG" error. This issue did not occur in version 3.10 and earlier.
To address this, I reviewed all the commits between v3.10.3...v3.11.0. One commit in particular stood out: in bc44dfc, @danielroe updated the
nitro.options._config.storageconfiguration for the pre-render cache (internal:nuxt:prerender).Before:
After:
Previously, the pre-render cache was stored in memory. With the updated configuration, the cache is now stored using a
cache-driverin the<rootDir>/.nuxt/cache/nitro/prerenderfolder.Upon further inspection of the
cache-drivercode, I discovered that when a pathname includes non-ASCII characters, it is used directly as a filename. This can lead to the "ENAMETOOLONG" error when the filename exceeds the 255-character limit. To resolve this, I added a call todecodeURIComponentto decode the pathnames before they are used, ensuring that filenames remain within acceptable length limits.