[almost done] Next.js 15 support #969
Conversation
|
WalkthroughThis set of changes restructures the codebase by removing deprecated or redundant packages, files, and configurations, especially those related to classic cache handling and server modules. The cache handler logic is refactored for improved type safety, modularity, and ES module compatibility. New utilities and configurations are introduced for TypeScript, ESLint, and Vitest. Redis and LRU cache handlers are modernized, and tests are updated or replaced. Several documentation and configuration files are updated or deleted to align with the new project structure. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant CacheHandler
participant LRUHandler
participant RedisHandler
participant RemoteStore
participant RedisClient
App->>CacheHandler: get(key, ctx)
CacheHandler->>LRUHandler: get(key, {implicitTags})
LRUHandler-->>CacheHandler: value/null
CacheHandler->>RedisHandler: get(key, {implicitTags})
RedisHandler-->>CacheHandler: value/null
CacheHandler-->>App: value/null
App->>CacheHandler: set(key, value, ctx)
CacheHandler->>LRUHandler: set(key, value)
LRUHandler-->>CacheHandler: ack
CacheHandler->>RedisHandler: set(key, value)
RedisHandler-->>CacheHandler: ack
CacheHandler-->>App: ack
App->>CacheHandler: revalidateTag(tag)
CacheHandler->>LRUHandler: revalidateTag(tag)
LRUHandler-->>CacheHandler: ack
CacheHandler->>RedisHandler: revalidateTag(tag)
RedisHandler-->>CacheHandler: ack
CacheHandler-->>App: ack
App->>RemoteStore: get(cacheKey)
RemoteStore->>RedisClient: get(key) with timeout
RedisClient-->>RemoteStore: value/null
RemoteStore-->>App: value/null
App->>RemoteStore: set(cacheKey, entry)
RemoteStore->>RedisClient: set(key, value) with timeout
RedisClient-->>RemoteStore: ack
RemoteStore-->>App: ack
App->>RemoteStore: expireTags(tags)
RemoteStore->>RedisClient: publish expired tags
RedisClient-->>RemoteStore: ack
RemoteStore-->>App: ack
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
apps/cache-testing/create-instances.ts (1)
36-55: Enhance performance and robustness.The main execution flow could be improved in several ways:
- Parallelize independent operations
- Make port numbers configurable
- Add cleanup and validation
Consider this improved implementation:
+const PORTS = process.env.INSTANCE_PORTS?.split(',') ?? ['3000', '3001']; + +// Clean up existing directories +async function cleanup() { + try { + await rm(INSTANCES_DIR, { recursive: true, force: true }); + } catch (error) { + console.warn('Cleanup failed:', error); + } +} + +// Validate directory contents +async function validateDir(dir: string): Promise<boolean> { + try { + await access(dir); + return true; + } catch { + return false; + } +} + +// Main execution +await cleanup(); + // Copy public directory to standalone app directory -await copyDir(PUBLIC_DIR, path.join(APP_DIR, 'public')); +const copyTasks = [ + copyDir(PUBLIC_DIR, path.join(APP_DIR, 'public')), + copyDir(STATIC_DIR, path.join(APP_DIR, '.next/static')), +]; + +const results = await Promise.all(copyTasks); +if (results.some(result => result)) { + console.error('One or more copy operations failed'); + process.exit(1); +} -// Copy static directory to standalone app/.next directory -await copyDir(STATIC_DIR, path.join(APP_DIR, '.next/static')); // Create fetch cache directory and copy await mkdir(path.join(APP_DIR, '.next/cache/fetch-cache'), { recursive: true }); await copyDir(FETCH_CACHE_DIR, path.join(APP_DIR, '.next/cache/fetch-cache')); // Create instance directories -await createInstanceDir('3000'); -await createInstanceDir('3001'); +await Promise.all(PORTS.map(port => createInstanceDir(port))); // Copy files from standalone directory to instance directories -await copyDir(path.join(STANDALONE_DIR, '.'), path.join(INSTANCES_DIR, '3000')); -await copyDir(path.join(STANDALONE_DIR, '.'), path.join(INSTANCES_DIR, '3001')); +await Promise.all( + PORTS.map(port => + copyDir(path.join(STANDALONE_DIR, '.'), path.join(INSTANCES_DIR, port)) + ) +); + +// Validate final state +const validations = await Promise.all( + PORTS.map(port => validateDir(path.join(INSTANCES_DIR, port))) +); + +if (validations.some(valid => !valid)) { + console.error('One or more instance directories failed validation'); + process.exit(1); +}
🧹 Nitpick comments (1)
apps/cache-testing/create-instances.ts (1)
7-13: Consider making paths configurable.The hardcoded paths could be made configurable through environment variables or a configuration file for better flexibility across different environments.
Example approach:
+import { config } from './config'; + // Set common paths -const STANDALONE_DIR = path.join(currentDir, '.next/standalone'); -const APP_DIR = path.join(STANDALONE_DIR, 'apps/cache-testing'); -const PUBLIC_DIR = path.join(currentDir, 'public'); -const STATIC_DIR = path.join(currentDir, '.next/static'); -const FETCH_CACHE_DIR = path.join(currentDir, '.next/cache/fetch-cache'); -const INSTANCES_DIR = path.join(currentDir, '.next/__instances'); +const STANDALONE_DIR = path.join(currentDir, config.paths.standalone ?? '.next/standalone'); +const APP_DIR = path.join(STANDALONE_DIR, config.paths.app ?? 'apps/cache-testing'); +const PUBLIC_DIR = path.join(currentDir, config.paths.public ?? 'public'); +const STATIC_DIR = path.join(currentDir, config.paths.static ?? '.next/static'); +const FETCH_CACHE_DIR = path.join(currentDir, config.paths.fetchCache ?? '.next/cache/fetch-cache'); +const INSTANCES_DIR = path.join(currentDir, config.paths.instances ?? '.next/__instances');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/cache-testing/cache-handler-redis-stack.cjs(2 hunks)apps/cache-testing/cache-handler-redis-stack.js(2 hunks)apps/cache-testing/cluster.config.js(1 hunks)apps/cache-testing/create-instances.sh(0 hunks)apps/cache-testing/create-instances.ts(1 hunks)apps/cache-testing/package.json(1 hunks)
💤 Files with no reviewable changes (1)
- apps/cache-testing/create-instances.sh
✅ Files skipped from review due to trivial changes (1)
- apps/cache-testing/cluster.config.js
🔇 Additional comments (4)
apps/cache-testing/package.json (2)
6-6: ES Module Declaration AddedThe addition of "type": "module" ensures that this package is now interpreted as an ECMAScript module. This change is crucial given the migration toward modern module syntax and should align well with the intended Next.js 15 support.
8-8: Build Script Updated to Use TypeScript Instance SetupThe build script has been updated from invoking a shell script to using "tsx ./create-instances.ts" after "next build". This change reflects the shift to a TypeScript-based setup for instance creation. Please verify that the new create-instances.ts file is correctly implemented and located as specified.
apps/cache-testing/cache-handler-redis-stack.js (1)
3-6: LGTM! Module syntax transition looks correct.The transition from CommonJS to ES modules is implemented correctly, with proper import statements and default export.
Let's verify the package compatibility with ES modules:
Also applies to: 89-89
✅ Verification successful
🌐 Web query:
Does @neshca/cache-handler support ES modules?💡 Result:
Yes, `@neshca/cache-handler` supports ES Modules (ESM). The library added explicit support for ES Modules in patch version 0.6.6 through the change `ddf957f`[3][7]. This allows developers to use ESM syntax (`import/export`) when configuring their cache handlers rather than being restricted to CommonJS (`require/module.exports`). Key implementation details: 1. Configuration files can use `.mjs` extensions for ESM syntax[1] 2. Dynamic imports are supported in instrumentation files[1] 3. The library handles both ESM and CommonJS configurations interchangeably[1][3] Example ESM configuration from the documentation[1]: ```javascript // cache-handler.mjs import { CacheHandler } from '@neshca/cache-handler'; // ...ESM-style configuration export default CacheHandler; ``` The package maintains compatibility with both module systems while recommending ESM for new implementations[3][7]. When using Next.js versions 13.5.1+, developers can choose either module format based on their project configuration[1][4]. Citations: - 1: https://caching-tools.github.io/next-shared-cache/installation - 2: https://caching-tools.github.io/next-shared-cache/ - 3: https://classic.yarnpkg.com/en/package/@neshca/cache-handler - 4: https://dev.to/rafalsz/scaling-nextjs-with-redis-cache-handler-55lh - 5: https://github.com/fortedigital/nextjs-cache-handler/blob/master/README.md - 6: https://github.com/vercel/next.js/discussions/57040 - 7: https://github.com/caching-tools/next-shared-cache/blob/canary/packages/cache-handler/CHANGELOG.md - 8: https://npmpackage.info/package/@neshca/cache-handler - 9: https://www.npmjs.com/package/@neshca/cache-handler/v/0.4.3 - 10: https://www.npmjs.com/~caching-tools
Module Syntax Transition Verified and Compatible
The ES module transition in this file is correctly implemented. The import/export changes align with ES module standards, and the web query confirms that
@neshca/cache-handlersupports ES modules (since patch version 0.6.6).
- Import statements updated from CommonJS (
require) to ES modules (import) correctly.- The default export statement is properly converted.
- Package compatibility with ES modules has been verified via the web query.
apps/cache-testing/cache-handler-redis-stack.cjs (1)
3-6: LGTM! CommonJS syntax transition looks correct.The CommonJS module syntax is implemented correctly, with proper handling of default exports using
.defaultproperty access.Let's verify if this dual-module approach is necessary for Next.js 15:
Also applies to: 89-89
✅ Verification successful
🌐 Web query:
Does Next.js 15 require dual CommonJS and ES module support?💡 Result:
Next.js 15 does not inherently require dual CommonJS/ES module support for all projects, but compatibility depends on your project's configuration and dependencies. Here's a breakdown of key considerations: ### Module Handling in Next.js 1. **Default Behavior** Next.js traditionally uses CommonJS (`require()`) for server-side configurations like `next.config.js`[3][7]. However: - You can enable ES modules (ESM) by setting `"type": "module"` in `package.json`[1][4] - File extensions (`.mjs` for ESM / `.cjs` for CommonJS) override package-level settings[6] 2. **Third-Party Dependencies** Issues arise when: - Using ESM-only packages with `require()` syntax[7][9] - Mixing module formats without proper configuration[5][8] Example error: ```bash Error [ERR_REQUIRE_ESM]: Must use import to load ES Module[7][9] ``` ### Solutions for Compatibility | Approach | Implementation | Use Case | |----------|----------------|----------| | Dynamic Imports | `const module = await import('esm-package')`[2][9] | Loading ESM packages in CommonJS contexts | | Transpilation | Use Babel/Webpack to convert ESM→CJS[1][4] | Legacy Node.js environments | | Dual Package Exports | Configure `package.json` exports field[1][6]: ```json "exports": { "import": "./esm-module.mjs", "require": "./cjs-module.cjs" } ``` | Library authors supporting both formats | ### Key Considerations - **Next.js Configuration** While native ESM support exists, some tooling (like Jest) may still require CommonJS[3][10] - **Node.js Versions** Full ESM support requires Node.js ≥14.8 for top-level await[2], with stable support since v13.2[1][4] - **TypeScript Users** Must configure `tsconfig.json` with `module: "esnext"` and `moduleResolution: "node16"`[3] Recent updates show Next.js moving toward better ESM integration, but many production setups still benefit from maintaining dual compatibility during transition periods[3][10]. The framework doesn't mandate dual support, but it becomes necessary when using mixed-module dependencies or targeting environments with legacy requirements. Citations: - 1: https://blog.logrocket.com/commonjs-vs-es-modules-node-js/ - 2: https://byteofdev.com/posts/how-to-use-esm/ - 3: https://github.com/vercel/next.js/discussions/32239 - 4: https://dev.to/logrocket/commonjs-vs-es-modules-in-nodejs-2eo1 - 5: https://answers.netlify.com/t/netlify-functions-es-module-error-when-using-paths-config/111942 - 6: https://nodejs.org/api/packages.html - 7: https://sentry.io/answers/next-js-err-require-esm/ - 8: https://event-driven.io/en/how_to_tackle_esmodules_compatibility_issues/ - 9: https://www.freecodecamp.org/news/how-to-bypass-es-modules-error-in-next-js/ - 10: https://nextjs.org/docs/messages/import-esm-externals
Dual Module Approach Verification
The web query confirms that Next.js 15 does not inherently require dual CommonJS/ES module support. In your file, the CommonJS syntax using the .cjs extension—including proper handling of default exports via the
.defaultproperty—is correct. While maintaining dual support can be beneficial for certain compatibility scenarios, it isn’t mandated by Next.js 15.No changes are necessary in this case.
| async function copyDir(src: string, dest: string) { | ||
| try { | ||
| await cp(src, dest, { recursive: true }); | ||
| } catch (error) { | ||
| console.error(`Failed to copy from ${src} to ${dest}:`, error); | ||
| process.exit(1); | ||
| } | ||
| } | ||
|
|
||
| async function createInstanceDir(port: string) { | ||
| try { | ||
| await mkdir(path.join(INSTANCES_DIR, port), { recursive: true }); | ||
| } catch (error) { | ||
| console.error( | ||
| `Failed to create ${path.join(INSTANCES_DIR, port)} directory:`, | ||
| error, | ||
| ); | ||
| process.exit(1); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and type safety.
The helper functions could benefit from:
- Better error handling without direct process exits
- More explicit TypeScript types
- More descriptive error messages
Consider this improved implementation:
-async function copyDir(src: string, dest: string) {
+type CopyError = {
+ code: string;
+ message: string;
+ source: string;
+ destination: string;
+};
+
+async function copyDir(src: string, dest: string): Promise<void | CopyError> {
try {
await cp(src, dest, { recursive: true });
} catch (error) {
- console.error(`Failed to copy from ${src} to ${dest}:`, error);
- process.exit(1);
+ return {
+ code: 'COPY_FAILED',
+ message: `Failed to copy directory: ${error instanceof Error ? error.message : 'Unknown error'}`,
+ source: src,
+ destination: dest,
+ };
}
}
-async function createInstanceDir(port: string) {
+async function createInstanceDir(port: string): Promise<void | CopyError> {
try {
await mkdir(path.join(INSTANCES_DIR, port), { recursive: true });
} catch (error) {
- console.error(
- `Failed to create ${path.join(INSTANCES_DIR, port)} directory:`,
- error,
- );
- process.exit(1);
+ return {
+ code: 'CREATE_DIR_FAILED',
+ message: `Failed to create instance directory: ${error instanceof Error ? error.message : 'Unknown error'}`,
+ source: INSTANCES_DIR,
+ destination: port,
+ };
}
}Then handle errors in the main execution flow:
const result = await copyDir(src, dest);
if (result) {
console.error(`${result.code}: ${result.message}`);
process.exit(1);
}
better-salmon
force-pushed
the
upgrade-next-js
branch
3 times, most recently
from
c70cfa2 to
cc9e820
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/cache-testing/create-instances.ts (2)
56-59: Improve error message for expected conditions.The current error message suggests a failure when the fetch cache directory doesn't exist, which is an expected condition. Consider using a more appropriate log level and message.
- console.error('No fetch cache files to copy'); + console.info('No fetch cache files found - skipping copy');
62-63: Consider making port numbers configurable.The port numbers are hardcoded. Consider making them configurable through environment variables or command-line arguments for better flexibility.
+const INSTANCE_PORTS = process.env.INSTANCE_PORTS?.split(',') ?? ['3000', '3001']; + // Create instance directories -await createInstanceDir('3000'); -await createInstanceDir('3001'); +for (const port of INSTANCE_PORTS) { + await createInstanceDir(port); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
.github/workflows/tests.yml(1 hunks)apps/cache-testing/cache-handler-redis-stack.cjs(2 hunks)apps/cache-testing/cache-handler-redis-stack.js(2 hunks)apps/cache-testing/cluster.config.js(1 hunks)apps/cache-testing/create-instances.sh(0 hunks)apps/cache-testing/create-instances.ts(1 hunks)apps/cache-testing/next.config.js(1 hunks)apps/cache-testing/package.json(1 hunks)apps/cache-testing/src/instrumentation.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/cache-testing/create-instances.sh
🚧 Files skipped from review as they are similar to previous changes (7)
- .github/workflows/tests.yml
- apps/cache-testing/next.config.js
- apps/cache-testing/src/instrumentation.ts
- apps/cache-testing/package.json
- apps/cache-testing/cluster.config.js
- apps/cache-testing/cache-handler-redis-stack.cjs
- apps/cache-testing/cache-handler-redis-stack.js
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: redis-strings
- GitHub Check: redis-stack
🔇 Additional comments (3)
apps/cache-testing/create-instances.ts (3)
1-14: LGTM! Well-structured imports and path handling.The code uses proper ES module imports and platform-agnostic path handling.
15-34: Improve error handling and type safety.The helper functions would benefit from better error handling and type safety as previously suggested.
66-67: Add overwrite protection for instance directories.The copy operations might silently overwrite existing files. Consider adding a check to prevent accidental overwrites or add a force flag.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Check if instance directories already contain files that might be overwritten # Check if instance directories exist and contain files for port in 3000 3001; do if [ -d ".next/__instances/$port" ]; then echo "Checking files in instance directory $port:" find ".next/__instances/$port" -type f -exec ls -l {} \; fi doneLength of output: 119
Action Required: Add Overwrite Protection to Instance Directories
The current check shows that the instance directories (3000 and 3001) exist but are empty, so no files were overwritten during the copy process. However, because an empty directory doesn't conclusively prove that overwriting won't occur when files are present, manual verification remains necessary if those directories could later contain files. Consider adding a check (or a force flag option) in the copy logic to avoid accidental overwrites when files already exist.
better-salmon
force-pushed
the
upgrade-next-js
branch
from
cc9e820 to
d1ccb08
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/server/package.json (1)
16-17: 💡 Verification agent🧩 Analysis chain
Review the
exportsfield configuration.The
exportsfield being set tonullis unusual and might cause issues with module resolution in modern Node.js versions. Consider defining proper export paths for your package entry points.Let me check the current best practices for package exports:
🌐 Web query:
What is the recommended way to configure package.json exports field for ESM packages?💡 Result:
To configure the
package.jsonexportsfield effectively for ESM packages while ensuring compatibility with CommonJS (CJS), follow these best practices:
Key Recommendations
Use conditional
exportssyntax
Define separate entry points for ESM (import) and CJS (require) under the"."key:{ "exports": { ".": { "import": "./dist/esm/index.mjs", "require": "./dist/cjs/index.cjs" } } }This ensures Node.js selects the correct format based on how the package is imported[1][3][4].
Avoid
"type": "module"for dual-mode packages
Omittingtypedefaults to CommonJS, preventing conflicts when supporting both ESM and CJS consumers[3][7].Explicitly declare accessible subpaths
After definingexports, all other internal paths become private. To expose specific files/submodules:{ "exports": { ".": { /* ... */ }, "./utils": { "import": "./dist/esm/utils.mjs", "require": "./dist/cjs/utils.cjs" }, "./package.json": "./package.json" } }This allows imports like
import utils from 'your-pkg/utils'[1][4][10].Include TypeScript types (if applicable)
For TypeScript users:{ "exports": { ".": { "types": "./dist/types/index.d.ts", "import": "./dist/esm/index.mjs", "require": "./dist/cjs/index.cjs" } } }
Advanced Configuration
Environment-specific builds
Use conditions likenode,browser, or custom environments:{ "exports": { ".": { "node": "./node-specific.js", "browser": "./browser-specific.js", "default": "./fallback.js" } } }Version compatibility
For packages supporting legacy Node.js versions (<12.7.0), retainmainas a fallback:{ "main": "./dist/cjs/index.cjs", "exports": { /* ... */ } }
Pitfalls to Avoid
- Never mix directory-level access with file-level exports[1]
- Ensure all exposed subpaths are explicitly declared[4]
- Test imports in both ESM (
import) and CJS (require()) contexts[3]For full implementation examples, see GitHub repositories like package-json-exports[3].
Citations:
- 1: https://commerce.nearform.com/blog/2021/node-esm-and-exports
- 2: https://dev.to/rameshpvr/export-vs-main-in-packagejson-eoj
- 3: https://snyk.io/blog/building-npm-package-compatible-with-esm-and-cjs-2024/
- 4: https://hirok.io/posts/package-json-exports
- 5: https://metrobundler.dev/docs/package-exports/
- 6: https://forums.meteor.com/t/how-to-import-npm-package-with-exports-field-in-package-json/57276
- 7: https://jeremyrichardson.dev/blog/saga_of_learning_exports_property_in_packagejson
- 8: webpack/webpack#9509
- 9: meteor/meteor#11727
- 10: https://webpack.js.org/guides/package-exports/
Action Required: Update the
exportsField and Verify Compatibility Settings
The current
"exports": nullconfiguration is problematic for module resolution with ESM. Instead, define explicit export paths. For example, if supporting both ESM and CommonJS:{ "exports": { ".": { "import": "./dist/esm/index.mjs", "require": "./dist/cjs/index.cjs" } } }If your package is purely ESM, you can define a simpler export such as:
{ "exports": { ".": "./dist/esm/index.mjs" } }Adjust the entry points to match your build output.
Additionally, ensure the
distTagsconfiguration accurately reflects your intended Next.js 15 support, as the current tags list support for Next.js 13.5 and 14.Confirm that the updated
@types/nodeversion (22.13.4) is compatible with Next.js 15 and any other dependency requirements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
.github/workflows/tests.yml(1 hunks)apps/cache-testing/cache-handler-redis-stack.cjs(2 hunks)apps/cache-testing/cache-handler-redis-stack.js(2 hunks)apps/cache-testing/cluster.config.js(1 hunks)apps/cache-testing/create-instances.sh(0 hunks)apps/cache-testing/create-instances.ts(1 hunks)apps/cache-testing/next.config.js(1 hunks)apps/cache-testing/package.json(2 hunks)apps/cache-testing/src/instrumentation.ts(1 hunks)docs/cache-handler-docs/package.json(1 hunks)internal/backend/package.json(1 hunks)internal/eslint-config/package.json(1 hunks)internal/next-common/package.json(1 hunks)internal/next-lru-cache/package.json(1 hunks)package.json(1 hunks)packages/cache-handler/package.json(1 hunks)packages/json-replacer-reviver/package.json(1 hunks)packages/server/package.json(1 hunks)
💤 Files with no reviewable changes (1)
- apps/cache-testing/create-instances.sh
✅ Files skipped from review due to trivial changes (8)
- internal/backend/package.json
- internal/next-common/package.json
- internal/next-lru-cache/package.json
- packages/json-replacer-reviver/package.json
- package.json
- packages/cache-handler/package.json
- internal/eslint-config/package.json
- docs/cache-handler-docs/package.json
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/cache-testing/src/instrumentation.ts
- apps/cache-testing/cache-handler-redis-stack.cjs
- apps/cache-testing/next.config.js
- apps/cache-testing/cluster.config.js
- apps/cache-testing/create-instances.ts
- .github/workflows/tests.yml
- apps/cache-testing/package.json
- apps/cache-testing/cache-handler-redis-stack.js
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: redis-strings
- GitHub Check: redis-stack
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/server/package.json (1)
39-42: Update distTags for Next.js 15 support.Since this PR is for Next.js 15 support, consider adding "next15" to the distTags array.
"distTags": [ "next13.5", "next14", + "next15" ]
packages/server/package.json
Outdated
| @@ -31,7 +31,7 @@ | |||
| "@repo/next-common": "workspace:*", | |||
| "@repo/next-lru-cache": "workspace:*", | |||
| "@repo/typescript-config": "workspace:*", | |||
| "@types/node": "22.13.1", | |||
| "@types/node": "22.13.4", | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify compatibility with Next.js 15.
Let's verify if this version of @types/node is compatible with Next.js 15 and check if there's a newer version available.
🏁 Script executed:
#!/bin/bash
# Check the latest @types/node version
npm view @types/node versions --json | jq '.[-1]'Length of output: 61
🌐 Web query:
What version of @types/node is required for Next.js 15?
💡 Result:
Next.js 15 requires using @types/node version 18.x or higher to align with its minimum Node.js requirement of v18.18[4][7]. While not explicitly stated in documentation, this follows from:
-
Node Version Alignment:
@types/nodeversions correlate with Node.js releases (e.g.,@types/node@18matches Node.js v18 APIs)[9].- Next.js v15 mandates Node ≥18.18[4], so matching
@types/node@18+ensures type compatibility.
-
Upgrade Practices:
- Always update
@typespackages alongside their runtime counterparts[1][8]. - Use
npm install --save-dev @types/node@latestfor latest type definitions[9].
- Always update
-
Troubleshooting:
- If encountering type errors post-upgrade, check for mismatched
@typesversions[10]. - Ensure
tsconfig.jsonincludes"node"incompilerOptions.types[2].
- If encountering type errors post-upgrade, check for mismatched
For full compatibility:
npm install --save-dev @types/node@^18Citations:
- 1: https://nextjs.org/docs/app/building-your-application/upgrading/version-15
- 2: https://nextjs.org/docs/pages/api-reference/config/typescript
- 3: What's the version policy for @types/node? vercel/next.js#32695
- 4: https://nextjs.org/blog/next-15
- 5: https://dev.to/dimeloper/whats-new-in-nextjs-15-new-hooks-turbopack-and-more-2lo8
- 6: Error when initializing Next.js project with TypeScript (v14.2.17 and v15) vercel/next.js#72563
- 7: https://nextjs.org/docs/app/building-your-application/upgrading/version-14
- 8: Should TypeScript be a devDependency? (It now shows up under dependencies...) vercel/next.js#44283
- 9: https://www.npmjs.com/package/@types/node
- 10: Cannot install dependencies after upgrading to Next 15 and React 19 RC vercel/next.js#72204
Next.js 15 Compatibility Update:
Although the current dependency "@types/node": "22.13.4" technically meets the minimum version requirement (≥18.x) for Next.js 15, the upgrade guidelines recommend aligning with Node 18 by using @types/node@^18. This ensures that the type definitions closely match the Node.js runtime (v18.18) expected by Next.js 15 and avoids potential mismatches from APIs introduced in later Node versions.
- Action Item: Consider changing the dependency in
packages/server/package.jsonto:"@types/node": "^18"
better-salmon
force-pushed
the
upgrade-next-js
branch
from
d1ccb08 to
52e1de3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/cache-testing/create-instances.ts (2)
61-64: Consider making port numbers configurable.The port numbers are hardcoded, which reduces flexibility and reusability.
Consider making them configurable through environment variables or command-line arguments:
+const DEFAULT_PORTS = ['3000', '3001']; +const ports = process.env.INSTANCE_PORTS?.split(',') || DEFAULT_PORTS; + // Create instance directories -await createInstanceDir('3000'); -await createInstanceDir('3001'); +for (const port of ports) { + await createInstanceDir(port); +}
65-68: Optimize performance with parallel operations.The copy operations for instance directories are executed sequentially. These operations could be parallelized for better performance.
Consider using
Promise.allto parallelize the operations:-// Copy files from standalone directory to instance directories -await copyDir(path.join(STANDALONE_DIR, '.'), path.join(INSTANCES_DIR, '3000')); -await copyDir(path.join(STANDALONE_DIR, '.'), path.join(INSTANCES_DIR, '3001')); +// Parallelize copying files from standalone directory to instance directories +await Promise.all( + ports.map(port => + copyDir(path.join(STANDALONE_DIR, '.'), path.join(INSTANCES_DIR, port)) + ) +);internal/eslint-config/next.js (2)
66-68: Consider using**for recursive ignore patterns.The current ignore patterns might not catch files in nested directories. Consider using
**instead of*:- ignores: ['node_modules/*', '.next/*'], + ignores: ['node_modules/**', '.next/**'],
10-14: Consider expanding configuration documentation.Since this is a significant change for Next.js 15 support, consider adding more detailed documentation about:
- The reason for switching to flat config
- Required ESLint/TypeScript versions
- Any breaking changes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
.github/workflows/tests.yml(1 hunks)apps/cache-testing/cache-handler-redis-stack.cjs(2 hunks)apps/cache-testing/cache-handler-redis-stack.js(2 hunks)apps/cache-testing/cluster.config.js(1 hunks)apps/cache-testing/create-instances.sh(0 hunks)apps/cache-testing/create-instances.ts(1 hunks)apps/cache-testing/next.config.js(1 hunks)apps/cache-testing/package.json(2 hunks)apps/cache-testing/src/instrumentation.ts(1 hunks)docs/cache-handler-docs/package.json(1 hunks)internal/backend/package.json(1 hunks)internal/eslint-config/next.js(2 hunks)internal/eslint-config/package.json(1 hunks)internal/next-common/package.json(1 hunks)internal/next-lru-cache/package.json(1 hunks)package.json(1 hunks)packages/cache-handler/package.json(1 hunks)packages/json-replacer-reviver/package.json(1 hunks)packages/server/package.json(1 hunks)turbo.json(1 hunks)
💤 Files with no reviewable changes (1)
- apps/cache-testing/create-instances.sh
🚧 Files skipped from review as they are similar to previous changes (16)
- internal/backend/package.json
- packages/json-replacer-reviver/package.json
- internal/next-lru-cache/package.json
- apps/cache-testing/src/instrumentation.ts
- apps/cache-testing/cache-handler-redis-stack.cjs
- internal/next-common/package.json
- apps/cache-testing/next.config.js
- packages/server/package.json
- .github/workflows/tests.yml
- apps/cache-testing/cluster.config.js
- package.json
- packages/cache-handler/package.json
- apps/cache-testing/package.json
- internal/eslint-config/package.json
- apps/cache-testing/cache-handler-redis-stack.js
- docs/cache-handler-docs/package.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: redis-strings
- GitHub Check: redis-stack
🔇 Additional comments (5)
turbo.json (1)
14-15: Environment Variable Addition VerificationThe new environment variable
"NEXT_PRIVATE_DEBUG_CACHE"has been correctly added next to"PPR_ENABLED"in the build task'senvarray. Please verify that this variable aligns with the expectations for debugging in Next.js 15. Also, ensure that any related documentation or downstream usage in your codebase references and properly handles this new variable.apps/cache-testing/create-instances.ts (2)
1-14: LGTM! Well-organized imports and constants.The imports are appropriate for the file operations, and the path constants are clearly named and properly organized. The use of
fileURLToPathensures ESM compatibility.
15-34: Improve error handling and type safety.The helper functions could benefit from better error handling and type safety improvements.
The previous review comment's suggestions for improved error handling, explicit TypeScript types, and more descriptive error messages are still valid and would enhance the code's robustness.
internal/eslint-config/next.js (2)
13-14:✅ Verification successful
Verify ESLint version compatibility.
The type change to
typescript-eslint.ConfigArrayindicates usage of ESLint's new flat config format. This requires ESLint 9.x and compatible plugins.
🏁 Script executed:
#!/bin/bash # Check ESLint and related package versions in package.json files echo "Checking ESLint version in package.json files..." fd --type f "package.json" --exec jq -r 'select(.dependencies.eslint or .devDependencies.eslint) | "ESLint version in \(.name): \(.dependencies.eslint // .devDependencies.eslint)"'Length of output: 430
ESLint Version Compatibility Confirmed
The type change to
typescript-eslint.ConfigArrayfor the updated Next.js configuration is correct. Our version check shows that all package.json files are using ESLint 9.20.1, which supports the new flat config format. No further changes are needed based on this verification.
15-25:✅ Verification successful
Verify Next.js specific globals.
The configuration includes Node.js globals but might need additional Next.js specific globals for full compatibility with Next.js 15.
🌐 Web query:
What are the required globals for Next.js 15 ESLint configuration?💡 Result:
Next.js 15's ESLint configuration handles required global variables automatically through its built-in plugin (
eslint-plugin-next) and recommended configurations. Here's what you need to know:
Core Configuration
Thenext/core-web-vitalsruleset (included by default) automatically configures:
- Environment: Node.js/ES modules
- JSX/React global recognition
- TypeScript support (if used)
Implicit Globals
No manual global declarations are needed for:
- React/JSX components (
React,JSX, etc.)- Web APIs (
fetch,Request,Response)- Node.js globals (
module,process,__dirname)Custom Global Additions
If using experimental features like Edge Runtime or additional APIs, extend your.eslintrcwith:{ "globals": { "EdgeRuntime": "readonly", "AsyncLocalStorage": "readonly" } }For most projects using
next/core-web-vitalsorplugin:@next/next/recommended, no explicit global configuration is required[1][2][5].Citations:
- 1: https://nextjs.org/docs/app/api-reference/config/eslint
- 2: https://www.danielfullstack.com/article/how-to-configure-eslint-with-nextjs-in-3-minutes
- 3: https://dev.to/jordanahaines/just-use-this-nextjs-eslint-configuration-540
- 4: https://nextjs.org/blog/next-15
- 5: https://www.freecodecamp.org/news/how-to-set-up-eslint-prettier-stylelint-and-lint-staged-in-nextjs/
- 6: https://nextjs.org/blog/next-15-rc2
- 7: vercel/next.js#71763
- 8: https://eslint.org/docs/latest/rules/global-require
- 9: https://www.youtube.com/watch?v=Mloeq7mvI00
- 10: vercel/next.js#73389
Next.js Globals Verification: No Additional Globals Required
The current configuration correctly merges the standard recommended ESLint settings with Node.js globals. As of Next.js 15, if you’re using the built-in ESLint plugin (e.g., via
next/core-web-vitalsorplugin:@next/next/recommended), all necessary Next.js globals (for React/JSX, web APIs, etc.) are automatically handled. There’s no need to manually add extra Next.js specific globals unless you're targeting experimental features (e.g., Edge Runtime).
- File:
internal/eslint-config/next.js- Lines: 15-25 – The code already applies
globals.nodevia spread. Ensure that your project also includes the appropriate Next.js ESLint configuration to leverage the automatic global setup.
| try { | ||
| // Copy fetch cache directory to standalone app/.next directory | ||
| await mkdir(path.join(APP_DIR, '.next/cache/fetch-cache'), { | ||
| recursive: true, | ||
| }); | ||
|
|
||
| const files = await readdir(FETCH_CACHE_DIR); | ||
|
|
||
| if (files.length > 0) { | ||
| await copyDir( | ||
| FETCH_CACHE_DIR, | ||
| path.join(APP_DIR, '.next/cache/fetch-cache'), | ||
| ); | ||
| } | ||
| } catch (_error) { | ||
| // Ignore errors - directory might not exist or be empty | ||
| console.error('No fetch cache files to copy'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent error handling approach.
The error handling for fetch cache operations differs from other operations in the script. While other operations exit on error, this section silently continues with a console error.
Consider standardizing the error handling approach across all operations:
try {
// Copy fetch cache directory to standalone app/.next directory
await mkdir(path.join(APP_DIR, '.next/cache/fetch-cache'), {
recursive: true,
});
const files = await readdir(FETCH_CACHE_DIR);
if (files.length > 0) {
await copyDir(
FETCH_CACHE_DIR,
path.join(APP_DIR, '.next/cache/fetch-cache'),
);
}
-} catch (_error) {
- // Ignore errors - directory might not exist or be empty
- console.error('No fetch cache files to copy');
+} catch (error) {
+ console.warn('No fetch cache files to copy:', error instanceof Error ? error.message : 'Unknown error');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| // Copy fetch cache directory to standalone app/.next directory | |
| await mkdir(path.join(APP_DIR, '.next/cache/fetch-cache'), { | |
| recursive: true, | |
| }); | |
| const files = await readdir(FETCH_CACHE_DIR); | |
| if (files.length > 0) { | |
| await copyDir( | |
| FETCH_CACHE_DIR, | |
| path.join(APP_DIR, '.next/cache/fetch-cache'), | |
| ); | |
| } | |
| } catch (_error) { | |
| // Ignore errors - directory might not exist or be empty | |
| console.error('No fetch cache files to copy'); | |
| } | |
| try { | |
| // Copy fetch cache directory to standalone app/.next directory | |
| await mkdir(path.join(APP_DIR, '.next/cache/fetch-cache'), { | |
| recursive: true, | |
| }); | |
| const files = await readdir(FETCH_CACHE_DIR); | |
| if (files.length > 0) { | |
| await copyDir( | |
| FETCH_CACHE_DIR, | |
| path.join(APP_DIR, '.next/cache/fetch-cache'), | |
| ); | |
| } | |
| } catch (error) { | |
| console.warn('No fetch cache files to copy:', error instanceof Error ? error.message : 'Unknown error'); | |
| } |
better-salmon
force-pushed
the
upgrade-next-js
branch
from
52e1de3 to
dd10f82
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pnpm-workspace.yaml (2)
1-5: Package list formatting looks correct.
The packages are now listed without quotes (except within comments) which is acceptable in YAML. Just ensure this style is consistent throughout the project.
6-9: Consistency in dependency string quoting.
The newonlyBuiltDependenciessection mixes quoted ('@biomejs/biome') and unquoted (esbuild,sharp) entries. For readability and maintainability, consider using a consistent approach for quoting all entries.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
apps/cache-testing/package.json(2 hunks)docs/cache-handler-docs/package.json(1 hunks)internal/backend/package.json(1 hunks)internal/eslint-config/package.json(1 hunks)internal/next-common/package.json(1 hunks)internal/next-lru-cache/package.json(1 hunks)package.json(1 hunks)packages/cache-handler/package.json(1 hunks)packages/json-replacer-reviver/package.json(1 hunks)packages/server/package.json(1 hunks)pnpm-workspace.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/next-common/package.json
- internal/backend/package.json
- internal/next-lru-cache/package.json
- packages/cache-handler/package.json
- packages/server/package.json
- packages/json-replacer-reviver/package.json
- apps/cache-testing/package.json
- internal/eslint-config/package.json
🔇 Additional comments (5)
package.json (2)
27-33: DevDependencies version updates are aligned.
The versions for@changesets/cli,prettier, andturbohave been bumped as expected in this effort. Please ensure these updates are compatible with dependent packages and your broader build process.
34-34: Updated packageManager specification.
ThepackageManagerfield now specifies[email protected]along with its lockfile checksum. Make sure that all developers and CI environments are aware of this update to prevent any version conflicts.docs/cache-handler-docs/package.json (3)
13-16: Dependency version bumps for Next.js and documentation tools.
The updates for"next","nextra", and"nextra-theme-docs"(to versions15.2.0and4.2.13respectively) support the Next.js 15 initiative. Confirm that the documentation build process (e.g., vianext buildandpagefind) continues to operate as expected.
17-18: Verify React dependency versions.
Thereactandreact-domdependencies are set to"19.0.0". Please double-check that these versions are intentional and fully compatible with the updated Next.js 15 features, as Next.js typically maintains closer alignment with stable React releases.
20-26: DevDependencies update enhances type safety and linting support.
The updates for@types/node,@types/react,@types/react-dom, andeslintwill help maintain code quality while aligning with the latest Next.js version. It’s advisable to run the full test suite to ensure no regression issues arise from these version bumps.
better-salmon
force-pushed
the
upgrade-next-js
branch
from
b6da01d to
cceb5a9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/dependabot.yml (1)
13-13: Broadening the Dependency Version Range forlru-cacheThe update from a fixed
11.x.xrange to['>=11.0.0']allows for any version oflru-cachestarting from 11.0.0. This change improves flexibility but also opens up the possibility of incompatibilities with future major versions. Please ensure that the codebase is compatible with newer releases oflru-cacheor consider adding tests/compatibility checks that can catch breaking changes early.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (58)
.changeset/config.json(0 hunks).github/dependabot.yml(1 hunks)apps/cache-testing/next.config.js(2 hunks)apps/cache-testing/src/app/app/no-params/dynamic-false/[slug]/page.tsx(1 hunks)apps/cache-testing/src/app/app/no-params/dynamic-true/[slug]/page.tsx(1 hunks)apps/cache-testing/src/app/app/no-params/ssr/200/page.tsx(1 hunks)apps/cache-testing/src/app/app/ppr/page.tsx(2 hunks)apps/cache-testing/src/app/app/randomHex/[length]/page.tsx(1 hunks)apps/cache-testing/src/app/app/with-params/dynamic-false/[slug]/page.tsx(2 hunks)apps/cache-testing/src/app/app/with-params/dynamic-true/[slug]/page.tsx(2 hunks)apps/cache-testing/src/app/app/with-params/nesh-cache/[slug]/page.tsx(0 hunks)apps/cache-testing/src/app/app/with-params/unstable-cache/[slug]/page.tsx(0 hunks)apps/cache-testing/src/app/layout.tsx(1 hunks)apps/cache-testing/src/components/cache-state-watcher.tsx(1 hunks)apps/cache-testing/src/components/pre-rendered-at.tsx(1 hunks)apps/cache-testing/src/components/restart-button.tsx(1 hunks)apps/cache-testing/src/components/revalidate-button.tsx(1 hunks)apps/cache-testing/src/pages/_app.tsx(1 hunks)apps/cache-testing/src/pages/api/pages-cached-api.ts(0 hunks)apps/cache-testing/src/pages/layout.tsx(1 hunks)apps/cache-testing/src/pages/pages/no-paths/ssr-with-cache/200.tsx(0 hunks)apps/cache-testing/src/pages/pages/randomHex/[length].tsx(1 hunks)apps/cache-testing/src/utils/common-app-page.tsx(1 hunks)apps/cache-testing/src/utils/common-pages-page.tsx(1 hunks)apps/cache-testing/src/utils/create-get-data.ts(2 hunks)apps/cache-testing/src/utils/create-pages-get-get-server-side-props-cached.ts(0 hunks)apps/cache-testing/tests/app.spec.ts(0 hunks)apps/cache-testing/tests/pages.spec.ts(0 hunks)biome.json(1 hunks)internal/eslint-config/next.js(3 hunks)internal/next-common/src/next-common.ts(3 hunks)internal/next-lru-cache/src/cache-types/next-cache-handler-value.ts(2 hunks)internal/next-lru-cache/tsconfig.json(1 hunks)package.json(2 hunks)packages/cache-handler/src/cache-handler.ts(6 hunks)packages/cache-handler/src/functions/functions.ts(0 hunks)packages/cache-handler/src/functions/nesh-cache.ts(0 hunks)packages/cache-handler/src/functions/nesh-classic-cache.ts(0 hunks)packages/cache-handler/src/handlers/server.ts(0 hunks)packages/cache-handler/src/instrumentation/register-initial-cache.ts(3 hunks)packages/cache-handler/tsconfig.json(1 hunks)packages/json-replacer-reviver/.npmignore(0 hunks)packages/json-replacer-reviver/CHANGELOG.md(0 hunks)packages/json-replacer-reviver/LICENSE(0 hunks)packages/json-replacer-reviver/README.md(0 hunks)packages/json-replacer-reviver/package.json(0 hunks)packages/json-replacer-reviver/src/json-replacer-reviver.test.ts(0 hunks)packages/json-replacer-reviver/src/json-replacer-reviver.ts(0 hunks)packages/json-replacer-reviver/tsconfig.json(0 hunks)packages/json-replacer-reviver/tsup.config.ts(0 hunks)packages/server/.npmignore(0 hunks)packages/server/CHANGELOG.md(0 hunks)packages/server/LICENSE(0 hunks)packages/server/README.md(0 hunks)packages/server/package.json(0 hunks)packages/server/src/server.ts(0 hunks)packages/server/tsconfig.json(0 hunks)packages/server/tsup.config.ts(0 hunks)
💤 Files with no reviewable changes (29)
- .changeset/config.json
- packages/server/README.md
- packages/server/LICENSE
- packages/json-replacer-reviver/tsconfig.json
- packages/server/.npmignore
- packages/json-replacer-reviver/tsup.config.ts
- packages/json-replacer-reviver/LICENSE
- packages/json-replacer-reviver/README.md
- packages/server/package.json
- apps/cache-testing/src/pages/api/pages-cached-api.ts
- packages/cache-handler/src/functions/functions.ts
- packages/json-replacer-reviver/package.json
- packages/server/CHANGELOG.md
- packages/server/tsconfig.json
- apps/cache-testing/tests/app.spec.ts
- packages/json-replacer-reviver/CHANGELOG.md
- apps/cache-testing/tests/pages.spec.ts
- apps/cache-testing/src/pages/pages/no-paths/ssr-with-cache/200.tsx
- apps/cache-testing/src/utils/create-pages-get-get-server-side-props-cached.ts
- apps/cache-testing/src/app/app/with-params/unstable-cache/[slug]/page.tsx
- packages/server/src/server.ts
- packages/cache-handler/src/functions/nesh-classic-cache.ts
- packages/json-replacer-reviver/src/json-replacer-reviver.test.ts
- apps/cache-testing/src/app/app/with-params/nesh-cache/[slug]/page.tsx
- packages/cache-handler/src/handlers/server.ts
- packages/cache-handler/src/functions/nesh-cache.ts
- packages/json-replacer-reviver/.npmignore
- packages/json-replacer-reviver/src/json-replacer-reviver.ts
- packages/server/tsup.config.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🪛 GitHub Actions: Check Docs Build
apps/cache-testing/src/app/layout.tsx
[error] 56-56: Type error: Unused '@ts-expect-error' directive.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: redis-strings
- GitHub Check: redis-stack
🔇 Additional comments (47)
biome.json (1)
55-55: Update Global Variable Declaration for React ContextThe change updates the "globals" array from
["JSX"]to["React"], which aligns with the transition towards a React-centric configuration—especially pertinent for Next.js 15 support. Please verify that this update is consistent with the rest of your project’s linting settings and that any references expecting the older "JSX" global are updated accordingly.internal/eslint-config/next.js (4)
13-15: Migration to ESLint's new "flat" configuration format.You've correctly updated the type annotation and export format to use TypeScript-ESLint's new flat configuration pattern, which aligns with modern ESLint practices introduced in ESLint v8.21.0+. This approach offers better extensibility and compatibility with Next.js 15.
19-25: Language options configuration looks good.Proper implementation of the new languageOptions format with Node.js globals merged in. This ensures consistency across the codebase for available global variables.
45-47: Good choice disabling React prop-types.Disabling 'react/prop-types' is appropriate for TypeScript projects since TypeScript's static type checking provides better prop validation than the runtime checks from prop-types.
69-84: Well-configured TypeScript unused variables rule.Excellent detailed configuration of the
@typescript-eslint/no-unused-varsrule. The setup properly handles the common convention of prefixing intentionally unused variables with underscore (_), while still enforcing checks on other variables.apps/cache-testing/next.config.js (2)
14-14: Verify impact of commented out outputFileTracingRoot.This configuration property was previously used for file tracing. Since it's now commented out, ensure this doesn't negatively impact the build process in Next.js 15, especially for the standalone output.
The commented line also used
import.meta.dirnamewhich may have been changed in Next.js 15's configuration approach. If this property is still needed, consider updating to use a compatible path resolution method.
5-5:✅ Verification successful
Extension change aligns with module standardization.
The update from
.mjsto.jsaligns with the broader effort to standardize module usage across the application as mentioned in the PR summary. Ensure that the referenced filecache-handler-redis-stack.jshas been properly converted to maintain compatibility.
🏁 Script executed:
#!/bin/bash # Verify the existence and syntax of the cache handler file if [ -f "apps/cache-testing/cache-handler-redis-stack.js" ]; then echo "✅ File exists" # Check for ES module syntax grep -E "export |import " "apps/cache-testing/cache-handler-redis-stack.js" || echo "⚠️ No ES module syntax found" else echo "❌ File not found: apps/cache-testing/cache-handler-redis-stack.js" fiLength of output: 426
Verified: Module Standardization and File Conversion are Correctly Implemented
The file
apps/cache-testing/cache-handler-redis-stack.jsexists and is confirmed to use ES module syntax as required. The change from.mjsto.jsis in line with our module standardization efforts and no compatibility issues were found.apps/cache-testing/src/components/restart-button.tsx (1)
5-5: Return type update looks goodThe change from
JSX.ElementtoReact.ReactNodeis appropriate and aligns with Next.js 15 compatibility requirements.React.ReactNodeis more flexible and allows for a wider range of return types.apps/cache-testing/src/utils/common-pages-page.tsx (1)
12-12: Return type update looks goodThe change from
JSX.ElementtoReact.ReactNodeis appropriate and aligns with Next.js 15 compatibility requirements. This matches the consistent pattern applied across other components.apps/cache-testing/src/pages/_app.tsx (1)
5-8: Return type update and improved formattingThe change from
JSX.ElementtoReact.ReactNodeis appropriate for Next.js 15 support. Additionally, the improved parameter formatting with each parameter on its own line enhances readability.apps/cache-testing/src/utils/common-app-page.tsx (1)
13-13: Return type update looks goodThe change from
JSX.ElementtoReact.ReactNodeis appropriate and aligns with Next.js 15 compatibility requirements. This maintains consistency with other components in the codebase.apps/cache-testing/src/components/revalidate-button.tsx (1)
19-19: Type broadening improves flexibility.The change from
JSX.ElementtoReact.ReactNodeas the return type is a good practice that allows for more flexibility in what the component can return.React.ReactNodeis more inclusive and can represent any renderable node type (elements, strings, numbers, arrays, null, etc.), whileJSX.Elementis limited to only React elements created via JSX.apps/cache-testing/src/app/app/no-params/ssr/200/page.tsx (1)
8-8: Return type broadening aligns with React best practices.Changing the return type from
Promise<JSX.Element>toPromise<React.ReactNode>is appropriate for this async component. This provides more flexibility while maintaining type safety, conforming to the pattern being applied across the codebase to standardize return types.apps/cache-testing/src/pages/pages/randomHex/[length].tsx (1)
53-53: Consistent type improvement across components.The return type change from
JSX.ElementtoReact.ReactNodefollows the same pattern applied to other components in this PR. This is a beneficial change that improves type flexibility while maintaining strong typing for the component's return value.apps/cache-testing/src/components/cache-state-watcher.tsx (1)
10-10: Return type broadening completes the pattern.This change completes the consistent update pattern across components by changing the return type from
JSX.ElementtoReact.ReactNode. This standardization improves flexibility while maintaining type safety throughout the codebase.apps/cache-testing/src/pages/layout.tsx (1)
7-9: Return type updated to React.ReactNode for broader type flexibilityThis change from
JSX.ElementtoReact.ReactNodealigns with Next.js 15 standards and provides greater flexibility for return values.React.ReactNodeallows for strings, numbers, arrays, and other React elements to be returned, not just JSX elements.apps/cache-testing/src/app/layout.tsx (1)
13-15: Return type updated to React.ReactNode for broader type flexibilityThis change from
JSX.ElementtoReact.ReactNodealigns with Next.js 15 standards and provides greater flexibility for return values.apps/cache-testing/src/components/pre-rendered-at.tsx (1)
5-8: Return type updated to React.ReactNode for broader type flexibilityThis change from
JSX.ElementtoReact.ReactNodeprovides greater flexibility for return values and maintains consistency with other component updates in the codebase.apps/cache-testing/src/app/app/no-params/dynamic-false/[slug]/page.tsx (3)
10-10: Updated PageParams type to support asynchronous route parametersThe
PageParamstype has been updated to handle route parameters as promises, which is likely required for Next.js 15 compatibility.
14-16: Return type updated to Promise<React.ReactNode> for broader type flexibilityThis change aligns with Next.js 15 standards and provides greater flexibility for return values by using
React.ReactNodeinstead ofJSX.Element.
17-18: Properly handling asynchronous route parametersThe code now correctly awaits the promise-based parameters before using them, addressing the type change made in the
PageParamsinterface.apps/cache-testing/src/app/app/ppr/page.tsx (1)
7-7: Type updates align with Next.js 15 standardsThese return type changes from
JSX.ElementtoReact.ReactNodeare appropriate and follow React best practices.React.ReactNodeis more flexible as it can represent any possible return value of a React component (elements, strings, numbers, arrays, etc.), whereasJSX.Elementis limited to React elements specifically.Also applies to: 23-23, 27-27
apps/cache-testing/src/app/app/no-params/dynamic-true/[slug]/page.tsx (1)
10-10: Correctly handles Promise-based params in Next.js 15The changes properly adapt the component to work with Next.js 15's Promise-based params. The type update to
Promise<{ slug: string }>and the addition of awaiting the params before accessing the slug property are necessary adaptations.Also applies to: 16-18
apps/cache-testing/src/utils/create-get-data.ts (1)
7-7: Streamlined caching implementation for Next.js 15The code has been simplified to use only standard
RequestCachetypes, removing the custom cache options ('unstable-cache' and 'nesh-cache'). The fetch implementation is now more direct with cleaner error handling, which aligns with Next.js 15's consolidated caching approach.Also applies to: 18-25, 27-28
apps/cache-testing/src/app/app/with-params/dynamic-false/[slug]/page.tsx (1)
6-6: Improved type definitions and Promise handling for Next.js 15The changes correctly adapt to Next.js 15's Promise-based params pattern. The explicit type definition for
generateStaticParamsimproves code clarity. The addition of awaiting params before accessing the slug property ensures compatibility with the new API.Also applies to: 14-18, 28-30
apps/cache-testing/src/app/app/with-params/dynamic-true/[slug]/page.tsx (3)
6-6: Updated type signature to handle asynchronous paramsThe
PageParamstype has been updated to makeparamsa Promise, which aligns with Next.js 15's new approach to handling route parameters asynchronously.
14-18: Explicit typing for generateStaticParams return valueThe return type is now more explicitly defined instead of using
PageParams['params'][], which improves type clarity and maintainability.
28-30: Updated component to handle async params and broadened return typeThe changes correctly:
- Update the return type to
React.ReactNodewhich is more flexible thanJSX.Element- Add async handling for params to accommodate the new Promise-based type structure
This implementation properly adapts to Next.js 15's new parameter handling approach.
apps/cache-testing/src/app/app/randomHex/[length]/page.tsx (3)
10-10: Updated type signature to handle asynchronous paramsThe
PageParamstype has been updated to makeparamsa Promise, consistent with Next.js 15's new approach to handling route parameters asynchronously.
12-16: Made generateStaticParams async with explicit return typeThe function has been properly updated to:
- Be marked as
async- Have a clearly defined return type instead of using
PageParams['params'][]This change is consistent with Next.js 15's parameter handling approach.
21-24: Updated component to handle async params and broadened return typeThe changes correctly:
- Update the destructuring to handle the Promise-based params
- Await the resolution of the params before accessing the length property
- Change the return type to the more flexible
React.ReactNodeThese updates appropriately adapt the component to Next.js 15's new parameter handling pattern.
internal/next-lru-cache/tsconfig.json (1)
4-7: Verify and Confirm Module Isolation SettingThe new
"compilerOptions"block introduces"isolatedModules": false. Please confirm that disabling isolated module checks is intentional for Next.js 15 support and aligns with the overall migration to ECMAScript modules. This configuration may affect how modules are transpiled and type-checked, so ensure that it meets your project’s requirements.packages/cache-handler/tsconfig.json (1)
5-6: Ensure Consistency with ECMAScript Module MigrationThe addition of
"isolatedModules": falsein thecompilerOptionssection matches changes in other parts of the repository. Please verify that this setting is consistent with the expectations of the new TypeScript build process under Next.js 15. Disabling isolated modules can bypass some module-level type checks, so double-check that this is the intended behavior.internal/next-lru-cache/src/cache-types/next-cache-handler-value.ts (4)
1-1: Import enhanced to include CachedRouteKind enumGood change to import the enum directly, providing better type safety than using string literals.
14-18: Improved type safety with CachedRouteKind enumReplacing string literals with enum values from CachedRouteKind improves code maintainability and provides better type checking.
Also applies to: 18-23, 24-27, 28-31
32-37: Added size calculation for PAGES and APP_PAGE route kindsThe addition of specific size calculations for these route kinds helps ensure accurate memory usage tracking. The implementation correctly accounts for both HTML content and associated data.
38-40:❓ Verification inconclusive
Verify the default case behavior
The default case now simply returns 0 instead of attempting to estimate size. This might lead to underestimating memory usage for unhandled cache types.
Is this intentional, or should there be a fallback calculation for unknown types?
🏁 Script executed:
#!/bin/bash # Look for any other cache kinds that might need specific size calculations ast-grep --pattern 'CachedRouteKind.$_' | grep -v "REDIRECT\|IMAGE\|FETCH\|APP_ROUTE\|PAGES\|APP_PAGE"Length of output: 104
Cache Type Fallback Behavior Verification
After running the search for additional cache kinds (beyond REDIRECT, IMAGE, FETCH, APP_ROUTE, PAGES, and APP_PAGE) we didn’t identify any extra entries in the codebase. Please confirm that having the default case simply return 0 is an intentional design decision. If there’s any possibility for new or unforeseen cache types in the future, consider if a fallback size estimation would prevent underestimating memory usage.
internal/next-common/src/next-common.ts (3)
8-11: Improved import organization for typesGood refactoring to improve type organization, separating CachedRouteKind and IncrementalCacheValue in a clean import structure.
27-27: Added export for CachedRouteKind enumExplicitly exporting CachedRouteKind makes it available for other modules, supporting the migration from string literals to enum values.
134-134: Updated IncrementalCachedPageValue to use enumThe type now references CachedRouteKind.PAGES instead of a string literal, improving type safety and consistency.
packages/cache-handler/src/instrumentation/register-initial-cache.ts (3)
3-8: Improved import organization and added CachedRouteKindThe imports are now better organized with named imports, and CachedRouteKind is properly imported.
208-208: Updated route cache to use CachedRouteKind.APP_ROUTEReplaced string literal 'ROUTE' with the enum value for consistency and type safety.
295-295: Updated page cache to use CachedRouteKind.PAGESReplaced string literal 'PAGE' with the enum value for consistency and type safety.
packages/cache-handler/src/cache-handler.ts (4)
16-16: Added import for CachedRouteKind enumGood addition to support the transition from string literals to enum values throughout the file.
442-442: Updated kind checks to use CachedRouteKind enumReplaced string literal comparisons with enum values, improving type safety and consistency.
Also applies to: 852-857
907-940: Refactored switch statement to use enum valuesAll case statements now use CachedRouteKind enum values instead of string literals, and the conditional check for pages has been updated accordingly.
832-832:❓ Verification inconclusive
Removed default value for ctx parameter
The default value for the
ctxparameter in thegetmethod has been removed, making it a required parameter.This could potentially break existing code if it relied on the default value. Please confirm that all call sites provide this parameter.
🏁 Script executed:
#!/bin/bash # Find all calls to get() method to check if they provide the ctx parameter ast-grep --pattern 'cacheHandler.get($_, $_)' || true ast-grep --pattern '\.get($_, $_)' | grep -v 'CacheHandler.#mergedHandler'Length of output: 122
Attention: Verify Explicit Provision of
ctxParameterThe default value for the
ctxparameter in thegetmethod (atpackages/cache-handler/src/cache-handler.tsline 832) has been removed, making it mandatory. Automated AST queries did not return any results, which might indicate that no call sites rely on the default value—but the absence of output is inconclusive.
- Action Needed: Manually verify that every call to the
get()method provides thectxparameter to prevent any potential runtime errors.
better-salmon
force-pushed
the
upgrade-next-js
branch
from
cceb5a9 to
8295b90
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/cache-testing/tsconfig.json (1)
8-8: Confirm the updatedincludearray.
The"include"array now omits"run-app-instances.ts", consolidating the entries into a single line. Please verify that excluding"run-app-instances.ts"is intentional and that no necessary files are skipped from type checking.packages/cache-handler/src/lru-cache-next-adapter/cache-types/next-cache-handler-value.ts (1)
42-42: Simplified default case to return 0.The default case now returns 0 instead of attempting to calculate a size. This might be less accurate if there are unhandled kinds in the future.
Consider adding a warning log in the default case to help identify any unhandled kinds during development, or implement a fallback calculation:
- return 0; + console.warn(`Unhandled kind in calculateObjectSize: ${(value as any).kind}`); + return JSON.stringify(value).length || 0;packages/cache-handler/package.json (1)
17-17: Trailing space in repository URL.There's an extra space at the end of the repository URL which should be removed.
- "url": "git+https://github.com/caching-tools/next-shared-cache.git " + "url": "git+https://github.com/caching-tools/next-shared-cache.git"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (94)
.changeset/config.json(0 hunks).github/dependabot.yml(1 hunks).github/workflows/tests.yml(1 hunks).gitignore(1 hunks)apps/cache-testing/cache-handler-redis-stack.cjs(2 hunks)apps/cache-testing/cache-handler-redis-stack.js(2 hunks)apps/cache-testing/next.config.ts(1 hunks)apps/cache-testing/package.json(2 hunks)apps/cache-testing/src/app/app/no-params/dynamic-false/[slug]/page.tsx(1 hunks)apps/cache-testing/src/app/app/no-params/dynamic-true/[slug]/page.tsx(1 hunks)apps/cache-testing/src/app/app/no-params/ssr/200/page.tsx(1 hunks)apps/cache-testing/src/app/app/ppr/page.tsx(2 hunks)apps/cache-testing/src/app/app/randomHex/[length]/page.tsx(1 hunks)apps/cache-testing/src/app/app/with-params/dynamic-false/[slug]/page.tsx(2 hunks)apps/cache-testing/src/app/app/with-params/dynamic-true/[slug]/page.tsx(2 hunks)apps/cache-testing/src/app/app/with-params/nesh-cache/[slug]/page.tsx(0 hunks)apps/cache-testing/src/app/app/with-params/unstable-cache/[slug]/page.tsx(0 hunks)apps/cache-testing/src/app/layout.tsx(1 hunks)apps/cache-testing/src/components/cache-state-watcher.tsx(1 hunks)apps/cache-testing/src/components/pre-rendered-at.tsx(1 hunks)apps/cache-testing/src/components/restart-button.tsx(1 hunks)apps/cache-testing/src/components/revalidate-button.tsx(1 hunks)apps/cache-testing/src/instrumentation.ts(1 hunks)apps/cache-testing/src/pages/_app.tsx(1 hunks)apps/cache-testing/src/pages/api/pages-cached-api.ts(0 hunks)apps/cache-testing/src/pages/layout.tsx(1 hunks)apps/cache-testing/src/pages/pages/no-paths/ssr-with-cache/200.tsx(0 hunks)apps/cache-testing/src/pages/pages/randomHex/[length].tsx(1 hunks)apps/cache-testing/src/utils/common-app-page.tsx(1 hunks)apps/cache-testing/src/utils/common-pages-page.tsx(1 hunks)apps/cache-testing/src/utils/create-get-data.ts(2 hunks)apps/cache-testing/src/utils/create-pages-get-get-server-side-props-cached.ts(0 hunks)apps/cache-testing/tests/app.spec.ts(0 hunks)apps/cache-testing/tests/pages.spec.ts(0 hunks)apps/cache-testing/tsconfig.json(1 hunks)biome.json(1 hunks)docs/cache-handler-docs/package.json(1 hunks)docs/cache-handler-docs/src/app/layout.tsx(0 hunks)docs/cache-handler-docs/tsconfig.json(1 hunks)internal/backend/package.json(2 hunks)internal/backend/tsconfig.json(1 hunks)internal/eslint-config/next.js(3 hunks)internal/eslint-config/package.json(1 hunks)internal/next-common/package.json(0 hunks)internal/next-common/tsconfig.json(0 hunks)internal/next-lru-cache/package.json(0 hunks)internal/next-lru-cache/tsconfig.json(0 hunks)internal/typescript-config/lib.json(1 hunks)internal/typescript-config/local.json(1 hunks)internal/typescript-config/nextjs.json(1 hunks)package.json(2 hunks)packages/cache-handler/.npmignore(1 hunks)packages/cache-handler/package.json(2 hunks)packages/cache-handler/src/cache-handler.ts(4 hunks)packages/cache-handler/src/functions/functions.ts(0 hunks)packages/cache-handler/src/functions/nesh-cache.ts(0 hunks)packages/cache-handler/src/functions/nesh-classic-cache.ts(0 hunks)packages/cache-handler/src/handlers/experimental-redis-cluster.ts(1 hunks)packages/cache-handler/src/handlers/local-lru.ts(1 hunks)packages/cache-handler/src/handlers/redis-stack.ts(1 hunks)packages/cache-handler/src/handlers/redis-strings.ts(6 hunks)packages/cache-handler/src/handlers/server.ts(0 hunks)packages/cache-handler/src/helpers/create-validated-age-estimation-function.test.ts(1 hunks)packages/cache-handler/src/helpers/create-validated-age-estimation-function.ts(1 hunks)packages/cache-handler/src/helpers/helpers.ts(1 hunks)packages/cache-handler/src/helpers/is-implicit-tag.ts(1 hunks)packages/cache-handler/src/helpers/is-tags-manifest.ts(1 hunks)packages/cache-handler/src/helpers/promise-with-timeout.test.ts(1 hunks)packages/cache-handler/src/helpers/promise-with-timeout.ts(1 hunks)packages/cache-handler/src/instrumentation/instrumentation.ts(0 hunks)packages/cache-handler/src/instrumentation/register-initial-cache.ts(3 hunks)packages/cache-handler/src/lru-cache-next-adapter/cache-types/cache-string-value.ts(1 hunks)packages/cache-handler/src/lru-cache-next-adapter/cache-types/next-cache-handler-value.ts(2 hunks)packages/cache-handler/src/next-common-types.ts(3 hunks)packages/cache-handler/tsconfig.json(1 hunks)packages/cache-handler/tsup.config.ts(0 hunks)packages/json-replacer-reviver/.npmignore(0 hunks)packages/json-replacer-reviver/CHANGELOG.md(0 hunks)packages/json-replacer-reviver/LICENSE(0 hunks)packages/json-replacer-reviver/README.md(0 hunks)packages/json-replacer-reviver/package.json(0 hunks)packages/json-replacer-reviver/src/json-replacer-reviver.test.ts(0 hunks)packages/json-replacer-reviver/src/json-replacer-reviver.ts(0 hunks)packages/json-replacer-reviver/tsconfig.json(0 hunks)packages/json-replacer-reviver/tsup.config.ts(0 hunks)packages/server/.npmignore(0 hunks)packages/server/CHANGELOG.md(0 hunks)packages/server/LICENSE(0 hunks)packages/server/README.md(0 hunks)packages/server/package.json(0 hunks)packages/server/src/server.ts(0 hunks)packages/server/tsconfig.json(0 hunks)packages/server/tsup.config.ts(0 hunks)turbo.json(2 hunks)
💤 Files with no reviewable changes (36)
- docs/cache-handler-docs/src/app/layout.tsx
- internal/next-common/tsconfig.json
- packages/json-replacer-reviver/LICENSE
- packages/server/README.md
- packages/server/LICENSE
- packages/server/package.json
- packages/json-replacer-reviver/.npmignore
- packages/json-replacer-reviver/tsup.config.ts
- packages/server/CHANGELOG.md
- packages/server/.npmignore
- packages/server/tsup.config.ts
- internal/next-lru-cache/tsconfig.json
- packages/json-replacer-reviver/package.json
- packages/server/tsconfig.json
- internal/next-lru-cache/package.json
- packages/cache-handler/tsup.config.ts
- packages/json-replacer-reviver/README.md
- internal/next-common/package.json
- packages/json-replacer-reviver/CHANGELOG.md
- apps/cache-testing/tests/app.spec.ts
- packages/cache-handler/src/functions/functions.ts
- apps/cache-testing/src/pages/api/pages-cached-api.ts
- apps/cache-testing/src/utils/create-pages-get-get-server-side-props-cached.ts
- packages/cache-handler/src/functions/nesh-classic-cache.ts
- packages/cache-handler/src/instrumentation/instrumentation.ts
- apps/cache-testing/src/app/app/with-params/unstable-cache/[slug]/page.tsx
- packages/cache-handler/src/handlers/server.ts
- .changeset/config.json
- apps/cache-testing/src/app/app/with-params/nesh-cache/[slug]/page.tsx
- packages/json-replacer-reviver/tsconfig.json
- packages/server/src/server.ts
- packages/json-replacer-reviver/src/json-replacer-reviver.test.ts
- apps/cache-testing/src/pages/pages/no-paths/ssr-with-cache/200.tsx
- apps/cache-testing/tests/pages.spec.ts
- packages/cache-handler/src/functions/nesh-cache.ts
- packages/json-replacer-reviver/src/json-replacer-reviver.ts
✅ Files skipped from review due to trivial changes (13)
- .gitignore
- packages/cache-handler/src/helpers/promise-with-timeout.ts
- packages/cache-handler/.npmignore
- packages/cache-handler/src/helpers/create-validated-age-estimation-function.ts
- packages/cache-handler/src/helpers/is-tags-manifest.ts
- packages/cache-handler/src/helpers/promise-with-timeout.test.ts
- packages/cache-handler/src/helpers/create-validated-age-estimation-function.test.ts
- internal/typescript-config/lib.json
- packages/cache-handler/src/helpers/helpers.ts
- packages/cache-handler/src/lru-cache-next-adapter/cache-types/cache-string-value.ts
- packages/cache-handler/src/handlers/redis-stack.ts
- packages/cache-handler/src/helpers/is-implicit-tag.ts
- packages/cache-handler/src/handlers/experimental-redis-cluster.ts
🚧 Files skipped from review as they are similar to previous changes (28)
- apps/cache-testing/src/components/cache-state-watcher.tsx
- apps/cache-testing/src/components/restart-button.tsx
- apps/cache-testing/src/components/revalidate-button.tsx
- apps/cache-testing/src/utils/common-app-page.tsx
- apps/cache-testing/src/app/app/no-params/ssr/200/page.tsx
- apps/cache-testing/src/pages/pages/randomHex/[length].tsx
- apps/cache-testing/src/pages/layout.tsx
- apps/cache-testing/src/components/pre-rendered-at.tsx
- apps/cache-testing/src/pages/_app.tsx
- .github/dependabot.yml
- apps/cache-testing/src/instrumentation.ts
- apps/cache-testing/cache-handler-redis-stack.cjs
- internal/backend/package.json
- turbo.json
- apps/cache-testing/src/utils/common-pages-page.tsx
- biome.json
- apps/cache-testing/src/app/app/no-params/dynamic-true/[slug]/page.tsx
- apps/cache-testing/src/app/app/ppr/page.tsx
- apps/cache-testing/cache-handler-redis-stack.js
- .github/workflows/tests.yml
- apps/cache-testing/src/app/app/randomHex/[length]/page.tsx
- apps/cache-testing/src/app/layout.tsx
- apps/cache-testing/src/app/app/with-params/dynamic-true/[slug]/page.tsx
- internal/eslint-config/package.json
- apps/cache-testing/src/app/app/with-params/dynamic-false/[slug]/page.tsx
- packages/cache-handler/src/instrumentation/register-initial-cache.ts
- docs/cache-handler-docs/package.json
- package.json
🔇 Additional comments (64)
docs/cache-handler-docs/tsconfig.json (2)
4-7: Paths Configuration Update: Transition to “cache-testing”.The paths mapping has been updated from its previous value (presumably
"cache-handler-docs/*") to"cache-testing/*": ["./src/*"]to reflect the shift in focus. Please ensure that all import paths in the rest of the project have been updated accordingly so that there are no unresolved module errors.
8-8: Update to "include" Array: Aligning with Next.js 15.The "include" array now includes
.next/types/**/*.tsand has removed entries likenext.config.tsto better match the Next.js 15 file structure. Verify that all necessary TypeScript files are still covered and that no critical files are inadvertently excluded from the compilation process.internal/backend/tsconfig.json (3)
2-2: Ensure TSConfig base configuration update is intentional.
The change from@repo/typescript-config/base.jsonto@repo/typescript-config/local.jsonindicates a switch to a configuration intended for local development or specific project needs. Verify that any differences in settings between these two files align with your current TypeScript and Next.js 15 requirements.
3-3: Confirm the include pattern covers all required files.
The updated"include": ["**/*.ts"]will match all TypeScript files. If your project (or parts of it) uses TSX files (especially in a Next.js context), consider whether they should also be included (e.g.,"**/*.{ts,tsx}").
4-4: Review exclusion settings for potential side effects.
Excluding only"node_modules"means that directories likedist(previously excluded) will now be processed. Verify that this change is intentional and that it won’t introduce unwanted files into the compilation process.internal/typescript-config/nextjs.json (2)
5-17: Modern TypeScript Compiler Options Setup for Next.jsThe updated options (target, lib, allowJs, skipLibCheck, strict, noEmit, esModuleInterop, module, moduleResolution, resolveJsonModule, isolatedModules, jsx, and incremental) are well-aligned with a modern Next.js workflow. Please double-check that using
"target": "ES2017"meets your application's browser support requirements and that"moduleResolution": "bundler"is fully supported by your build pipeline.
18-22: Next.js Plugin Declaration ValidatedThe plugins array correctly retains the Next.js plugin configuration. This minimal setup is appropriate; however, ensure that if additional plugins are needed later they follow the same structured schema.
apps/cache-testing/tsconfig.json (1)
3-7: Review thepathsconfiguration.
The"paths"property mapping"cache-testing/*"to"./src/*"is clear and concise. There are no apparent issues here.packages/cache-handler/tsconfig.json (3)
1-3: Validate schema addition and extends update.
Adding the"$schema": "https://json.schemastore.org/tsconfig"property enhances editor support and configuration validation. Updating the"extends"property to use"@repo/typescript-config/lib.json"aligns with the new configurations. Please confirm that these changes are consistent with the intended configuration for packages in this repository.
4-7: Review compiler options configuration.
The addition of"outDir": "dist"and"rootDir": "src"in thecompilerOptionsensures that the build artifacts and source files are organized as expected. This setup is standard for library packages; no issues are observed here.
8-9: Confirm file inclusion and exclusion patterns.
The updated"include": ["**/*.ts"]ensures that only TypeScript files are processed, while"exclude": ["**/*.test.ts", "node_modules", "dist"]explicitly omits test files and build outputs. This configuration should help maintain a clean build but ensure that no essential TSX or other file types are inadvertently omitted if they are needed.internal/typescript-config/local.json (6)
3-4: Strengthen Node configuration with display and version properties.
The addition of"display": "Node 22"and"_version": "22.0.0"clearly signals the target environment and internal versioning for this configuration. Make sure that any tooling consuming this config recognizes and appropriately handles these fields.
6-7: Modernize target and library settings.
Changing"target"to"es2022"and updating"lib"to["es2024"]ensures that the build is aligned with the latest ECMAScript features expected in your Node 22 environment. Verify that all downstream dependencies are compatible with these settings.
11-11: Preserve module syntax for ES module compatibility.
The new setting"module": "preserve"maintains the original module syntax, which is essential when leveraging native ES modules in Node 22. Confirm that build tools and bundlers in the codebase are configured to work with this preserved module structure.
13-13: Enable strict type-checking.
Enabling"strict": trueenforces rigorous type safety, which will help catch errors early. This is a best practice in modern TypeScript projects.
15-15: Require explicit override declarations.
The introduction of"noImplicitOverride": trueensures that any method overrides are clearly marked, reducing potential mistakes in object-oriented code structures.
17-19: Improve module resolution and isolation.
Adding"resolveJsonModule": true,"isolatedModules": true, and"verbatimModuleSyntax": trueenhances module handling by:
- Allowing JSON imports with type safety.
- Enforcing isolation to aid tooling and transpilation.
- Preserving the original module syntax without transformation.
Confirm that your current TypeScript version and build environment fully support these options.internal/eslint-config/next.js (5)
13-15: Updated ESLint configuration to use flat config formatThe migration from traditional ESLint configuration array to the new flat config format using
tseslint.config()is a significant upgrade. This change aligns with modern ESLint practices and better supports TypeScript integration.
19-25: Well-structured language options configurationAdding explicit language options with proper globals from Node.js environment ensures consistent behavior across different environments where the code might run.
45-47: Appropriate rule disabling for TypeScript projectsDisabling 'react/prop-types' is appropriate since TypeScript's static typing eliminates the need for React's PropTypes validation.
69-84: Comprehensive unused variables configurationThis detailed configuration for
@typescript-eslint/no-unused-varsprovides a good balance between catching genuinely unused variables while allowing for common patterns like ignored parameters (prefixed with underscore).The configuration properly handles different contexts where variables might be intentionally unused:
- Function parameters
- Catch clause parameters
- Destructured array elements
- Regular variables
- Rest siblings in object destructuring
1-85:✅ Verification successful
Verify compatibility with the project's TypeScript version
The ESLint configuration is now using typescript-eslint's flat config format, which requires a specific minimum version of TypeScript and ESLint.
🏁 Script executed:
#!/bin/bash # Check TypeScript and ESLint versions in the project echo "Checking package.json for TypeScript and ESLint versions:" find . -name "package.json" -not -path "*/node_modules/*" -exec grep -l "typescript\|eslint" {} \; | xargs cat | grep -E '"typescript"|"eslint"' | sort | uniqLength of output: 363
Project Compatibility Verified: TypeScript 5.7.3 and ESLint 9.21.0 Meet Requirements
The package.json output confirms that the project is using TypeScript 5.7.3 and ESLint 9.21.0. These versions satisfy the minimum requirements for the typescript-eslint flat config format, so no changes are required based on this review comment.
apps/cache-testing/src/app/app/no-params/dynamic-false/[slug]/page.tsx (3)
10-10: Type update properly handles asynchronous params in Next.js 15The type update from
{ params: { slug: string } }to{ params: Promise<{ slug: string }> }aligns with Next.js 15's changes to how route parameters are handled.
16-16: Return type change provides better type flexibilityChanging from
Promise<JSX.Element>toPromise<React.ReactNode>is more appropriate as it's the recommended return type for React components, offering better compatibility with different node types.
17-18: Correctly handles asynchronous paramsThe code properly awaits the Promise-based params and uses the resolved value, which is the correct approach for Next.js 15's new parameter handling mechanism.
apps/cache-testing/src/utils/create-get-data.ts (5)
7-7: Simplified cache parameter typeThe
cacheparameter type has been simplified fromRequestCache | 'unstable-cache' | 'nesh-cache'to justRequestCache, indicating the removal of custom caching strategies. This aligns with Next.js 15's improved built-in caching.
18-21: Improved data fetching implementationThe implementation has been simplified to use the native
fetchAPI with Next.js 15's caching options, removing complex custom caching logic. This approach is cleaner and leverages platform standards.
23-25: Streamlined error handlingError handling has been simplified to a single check on the fetch response, which is sufficient for this use case and makes the code more maintainable.
27-28: Direct response parsingParsing the JSON response directly from the fetch result simplifies the code by eliminating unnecessary intermediate variables and handling.
4-37:❓ Verification inconclusive
Verify dependent files for cache removal compatibility
The removal of custom caching options ('unstable-cache' and 'nesh-cache') could impact dependent files that might still be using these options.
🏁 Script executed:
#!/bin/bash # Check for any usages of removed caching options in dependent files echo "Checking for usages of removed caching options..." rg -l "'unstable-cache'|'nesh-cache'" --type=ts --type=tsx apps/cache-testing/Length of output: 229
Action Required: Verify Dependent Files for Removed Cache Options
It looks like the initial shell command failed due to an unrecognized file type ("tsx"). To ensure that no dependent files in the
apps/cache-testing/directory are still referencing the removed caching options, please run the updated verification script below:#!/bin/bash # Define file type for tsx and search for usages of removed caching options in .ts and .tsx files rg --type-add 'tsx:*.tsx' -l "'unstable-cache'|'nesh-cache'" -g "*.{ts,tsx}" apps/cache-testing/Once executed, manually verify that the search results return no hits. If any files are listed, please update them to remove or replace references to
'unstable-cache'or'nesh-cache'.apps/cache-testing/next.config.ts (4)
2-2: Type import added for stronger typing.The addition of the NextConfig type enhances type safety in the configuration.
4-4: Path updated from .mjs to .js for Next.js 15 compatibility.This change aligns with the module system transitions mentioned in the PR objectives, moving from ES module syntax (.mjs) to regular JavaScript files (.js).
6-6: Added explicit type annotation for the config object.The configuration object is now properly typed as NextConfig, improving type safety and code clarity.
12-12: Commented out outputFileTracingRoot that used import.meta.dirname.This configuration may need to be updated with a compatible approach for Next.js 15 if this functionality is still required.
Would you like me to verify if there's a recommended replacement for this configuration in Next.js 15?
packages/cache-handler/src/handlers/local-lru.ts (2)
1-2: Imports updated to use relative paths with .js extensions.These changes replace package imports with relative paths, improving module resolution in ES modules.
4-5: Import paths updated to include .js extensions.Adding explicit .js extensions to import paths is a common pattern for ES module compatibility, which aligns with Next.js 15 requirements.
packages/cache-handler/src/cache-handler.ts (7)
15-16: Updated imports with .js extensions and added CachedRouteKind.Adding the CachedRouteKind enum import and using .js extensions in import paths improves type safety and module compatibility.
18-19: Updated helper function imports with .js extensions.Consistent use of .js extensions in import paths for ES module compatibility.
442-442: Replaced string literal with enum value.Using
CachedRouteKind.PAGESinstead of the string literal 'PAGE' improves type safety and maintainability.
897-904: Updated conditional checks to use enum values.Using
CachedRouteKindenum values instead of string literals improves type safety and makes the code more robust against typos.
910-910: Simplified value assignment.Direct assignment of incrementalCacheValue without modification simplifies the code.
915-918: Updated conditional check to use enum value.Consistent use of
CachedRouteKind.PAGESenum value instead of string literals improves type safety.
832-832:❓ Verification inconclusive
Default parameter value removed.
Removing the default value for the
ctxparameter makes it a required parameter. Ensure all calling code passes this parameter to avoid runtime errors.
🏁 Script executed:
#!/bin/bash # Search for calls to get method without providing ctx parameter rg -A 5 'get\s*\(\s*[^,)]+\s*\)' --type tsLength of output: 3661
Action Required: Ensure All Calls to
CacheHandler#getExplicitly ProvidectxThe change removing the default value for the
ctxparameter leaves it as required. Our search for invocations using a lone parameter did not reveal any obvious cases where the new required argument is omitted. However, since the removal of the default could lead to runtime errors if any call sites were overlooked, please review all usages ofCacheHandler#get—especially in less obvious contexts or integration flows—to ensure that a validctxvalue is passed every time.
- Confirm that every call to the
getmethod in the cache handler now correctly supplies thectxparameter.- Re-run integration tests to catch any potential runtime issues related to this change.
packages/cache-handler/src/lru-cache-next-adapter/cache-types/next-cache-handler-value.ts (3)
2-5: Updated imports for better structure and type safety.The import for CacheHandlerValue now also includes CachedRouteKind enum, which is used throughout the file for type-safe comparisons.
7-8: Updated import paths with .js extensions.Consistent use of .js extensions in import paths for ES module compatibility.
17-40: Updated switch cases to use enum values and added specific cases.Using
CachedRouteKindenum values instead of string literals improves type safety. Adding specific cases for PAGES and APP_PAGE with tailored size calculations improves accuracy.packages/cache-handler/src/next-common-types.ts (6)
5-5: Added CacheHandlerContext import for stronger typing.Good addition of the CacheHandlerContext type import, which is now used for the FileSystemCacheContext type definition below.
8-11: Updated imports with explicit .js extensions.The import paths now include
.jsextensions, which aligns with Next.js 15's ESM support requirements. This is a necessary change for compatibility.
14-14: Added explicit CacheHandler type export.Good addition to explicitly export the CacheHandler type, improving the module's public API.
22-24: Updated export paths with .js extensions and added CachedRouteKind.The export with explicit
.jsextension and adding the CachedRouteKind enum export improves compatibility with Next.js 15's ESM requirements.
90-95: Improved FileSystemCacheContext type definition.Good refactoring of the FileSystemCacheContext type to use Omit with CacheHandlerContext, which makes the type relationship more explicit while ensuring serverDistDir is properly typed as string.
134-134: Replaced string literal with enum value.Replacing the string literal 'PAGE' with the enum value CachedRouteKind.PAGES improves type safety and maintainability.
packages/cache-handler/src/handlers/redis-strings.ts (4)
1-1: Added superjson for improved serialization.Good addition of superjson, which provides better serialization/deserialization support than native JSON, especially for complex objects and special types like Date, Map, and Set.
3-8: Updated import paths with .js extensions.The import paths now include
.jsextensions, aligning with Next.js 15's ESM support requirements.
71-71: Replaced JSON.parse with superjson.parse.Good upgrade from JSON.parse to superjson.parse for better type preservation during deserialization.
119-119: Replaced JSON.stringify with superjson.stringify.Good upgrade from JSON.stringify to superjson.stringify for better type preservation during serialization.
Also applies to: 132-132, 157-157, 192-192
apps/cache-testing/package.json (4)
6-6: Added type:module for ESM support.Adding "type": "module" is necessary for Next.js 15 compatibility, which uses ESM by default.
8-9: Updated build script and added type checking.Good improvements:
- Changed build script to use TypeScript instead of a shell script
- Added a dedicated type checking script
These changes improve the development experience and maintainability.
21-24: Updated to Next.js 15 and React 19.Key dependency updates:
- Next.js 14.2.15 → 15.2.0
- React 18.3.1 → 19.0.0
- React DOM 18.3.1 → 19.0.0
These updates are essential for the PR's objective of supporting Next.js 15.
31-36: Updated dev dependencies and added dotenv-cli.Good updates to development tools and dependencies to maintain compatibility with the latest versions.
packages/cache-handler/package.json (4)
26-29: Simplified exports configuration for ESM.Good simplification of the exports field to use direct paths with
.jsextensions, which is more compatible with Next.js 15's ESM support.
32-34: Switched from tsup to tsc for builds.Updated build scripts to use TypeScript's tsc compiler directly instead of tsup. This is a reasonable approach for simple TypeScript projects, though tsup can offer faster builds for larger projects.
39-41: Added superjson dependency and updated others.Adding superjson as a dependency is consistent with the code changes in the handlers that now use it for improved serialization. Good use of caret ranges for semver compatibility.
50-50: Updated Next.js peer dependency range.Updated the peer dependency to require Next.js 15, which is appropriate given the codebase changes specifically targeting Next.js 15 compatibility.
better-salmon
force-pushed
the
upgrade-next-js
branch
2 times, most recently
from
5a174d8 to
1421a1c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
.vscode/settings.json (1)
1-44: Overall Configuration Cleanup and Modernization
The updated settings file demonstrates a thoughtful cleanup:
- The explicit setting for the TypeScript SDK supports improved type safety and aligns with the ES module migration.
- Removal of outdated configuration settings (file nesting patterns and Grammarly selectors) streamlines the VS Code environment.
Ensure that these removals won’t adversely affect any specific developer tooling requirements.
packages/cache-handler/src/helpers/create-redis-timeout-config.ts (1)
25-31: Consider adding input validation for negative timeout values.The function handles the case where
timeoutMsis 0, but there's no check for negative values. WhileAbortSignal.timeout()might handle negative values internally, adding explicit validation would make the function more robust and prevent potential issues.export function createRedisTimeoutConfig(timeoutMs: number): CommandOptions { + if (timeoutMs < 0) { + throw new Error('Timeout value cannot be negative'); + } if (timeoutMs === 0) { return commandOptions({}); } return commandOptions({ signal: AbortSignal.timeout(timeoutMs) }); }packages/cache-handler/src/utils/get-tags-from-headers.ts (1)
23-35: Consider handling edge cases more explicitlyThe implementation handles array and string cases well but could be more explicit about other potential header value types.
Consider adding explicit handling for other potential header value types:
export function getTagsFromHeaders(headers: OutgoingHttpHeaders): string[] { const tagsHeader = headers['x-next-cache-tags']; if (Array.isArray(tagsHeader)) { return tagsHeader; } if (typeof tagsHeader === 'string') { return tagsHeader.split(','); } + // Handle potential buffer or number types that might come from headers + if (tagsHeader !== undefined && tagsHeader !== null) { + // Convert to string and handle as string + return String(tagsHeader).split(','); + } return []; }packages/cache-handler/src/handlers/local-lru.ts (3)
1-7: Sort the import statements to fix pipeline failure.The pipeline reports that the import statements should be sorted. Adopting a consistent ordering practice generally places external libraries first, followed by local imports in alphabetical order. Consider applying this diff:
-import type { Handler } from '../cache-handler.js'; -import { NEXT_CACHE_IMPLICIT_TAG_ID } from '../next-common-types.js'; -import { - type CacheHandlerValue, - CachedRouteKind, -} from '../next-common-types.js'; -import { LRUCache } from 'lru-cache'; +import { LRUCache } from 'lru-cache'; +import type { Handler } from '../cache-handler.js'; +import { CachedRouteKind, type CacheHandlerValue, NEXT_CACHE_IMPLICIT_TAG_ID } from '../next-common-types.js';🧰 Tools
🪛 GitHub Actions: Tests
[error] 1-1: Import statements could be sorted.
9-44: Consider caching stringified data to reduce repeated JSON.stringify calls.Each case calculates size by calling
JSON.stringify(), which can be computationally expensive for large objects or frequent calls. A small optimization would be to store the stringified data in a local variable before returning its length:-function calculateObjectSize({ value }: CacheHandlerValue): number { - // Return default size if value is falsy - if (!value) { - return 25; - } - - switch (value.kind) { - case CachedRouteKind.REDIRECT: { - return JSON.stringify(value.props).length; - } - ... - } -} +function calculateObjectSize({ value }: CacheHandlerValue): number { + if (!value) { + return 25; + } + + switch (value.kind) { + case CachedRouteKind.REDIRECT: { + const strProps = JSON.stringify(value.props); + return strProps.length; + } + ... + } +}
132-197: Optimize revalidateTag for large caches.Currently, the cache iteration in
revalidateTag(for (const [key, { tags }] of lruCacheStore.entries())) can become expensive when the cache grows large. Consider maintaining a tag-to-keys index or reference counting to avoid scanning every entry:- for (const [key, { tags }] of lruCacheStore.entries()) { - if (tags.includes(tag)) { - lruCacheStore.delete(key); - } - } + // Potential approach (requires maintaining a separate Map or multi-map of tag -> set of keys) + const keysWithTag = tagsToKeysIndex.get(tag) || []; + for (const key of keysWithTag) { + lruCacheStore.delete(key); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
.vscode/settings.json(2 hunks)apps/cache-testing/package.json(2 hunks)apps/cache-testing/run-app-instances.ts(1 hunks)docs/cache-handler-docs/src/app/usage/creating-a-custom-handler/page.mdx(2 hunks)internal/typescript-config/test.json(1 hunks)packages/cache-handler/package.json(2 hunks)packages/cache-handler/src/cache-handler.ts(5 hunks)packages/cache-handler/src/handlers/experimental-redis-cluster.ts(9 hunks)packages/cache-handler/src/handlers/local-lru.ts(1 hunks)packages/cache-handler/src/handlers/redis-stack.ts(8 hunks)packages/cache-handler/src/handlers/redis-strings.ts(11 hunks)packages/cache-handler/src/helpers/create-redis-timeout-config.ts(1 hunks)packages/cache-handler/src/helpers/create-validated-age-estimation-function.test.ts(0 hunks)packages/cache-handler/src/helpers/create-validated-age-estimation-function.ts(0 hunks)packages/cache-handler/src/helpers/get-tags-from-headers.ts(0 hunks)packages/cache-handler/src/helpers/get-timeout-redis-command-options.ts(0 hunks)packages/cache-handler/src/helpers/helpers.ts(1 hunks)packages/cache-handler/src/helpers/is-implicit-tag.ts(0 hunks)packages/cache-handler/src/helpers/is-tag-implicit.ts(1 hunks)packages/cache-handler/src/helpers/is-tags-manifest.ts(0 hunks)packages/cache-handler/src/helpers/promise-with-timeout.test.ts(0 hunks)packages/cache-handler/src/helpers/promise-with-timeout.ts(0 hunks)packages/cache-handler/src/instrumentation/register-initial-cache.ts(3 hunks)packages/cache-handler/src/next-common-types.ts(3 hunks)packages/cache-handler/src/utils/compose-age-estimation-fn.ts(1 hunks)packages/cache-handler/src/utils/get-tags-from-headers.ts(1 hunks)packages/cache-handler/test/tsconfig.json(1 hunks)packages/cache-handler/test/utils/compose-age-estimation-fn.test.ts(1 hunks)packages/cache-handler/tsconfig.json(1 hunks)packages/cache-handler/vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (8)
- packages/cache-handler/src/helpers/promise-with-timeout.ts
- packages/cache-handler/src/helpers/get-tags-from-headers.ts
- packages/cache-handler/src/helpers/create-validated-age-estimation-function.test.ts
- packages/cache-handler/src/helpers/create-validated-age-estimation-function.ts
- packages/cache-handler/src/helpers/is-tags-manifest.ts
- packages/cache-handler/src/helpers/promise-with-timeout.test.ts
- packages/cache-handler/src/helpers/get-timeout-redis-command-options.ts
- packages/cache-handler/src/helpers/is-implicit-tag.ts
✅ Files skipped from review due to trivial changes (4)
- packages/cache-handler/test/tsconfig.json
- apps/cache-testing/run-app-instances.ts
- packages/cache-handler/vitest.config.ts
- internal/typescript-config/test.json
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/cache-handler/src/next-common-types.ts
- packages/cache-handler/src/handlers/redis-strings.ts
- packages/cache-handler/src/handlers/experimental-redis-cluster.ts
- packages/cache-handler/src/handlers/redis-stack.ts
- packages/cache-handler/src/instrumentation/register-initial-cache.ts
- packages/cache-handler/package.json
- apps/cache-testing/package.json
🧰 Additional context used
🪛 GitHub Actions: Tests
packages/cache-handler/test/utils/compose-age-estimation-fn.test.ts
[error] 3-3: Formatter would have printed the following content: import { composeAgeEstimationFn, getInitialExpireAge } from '../../src/utils/compose-age-estimation-fn.js';
packages/cache-handler/src/handlers/local-lru.ts
[error] 1-1: Import statements could be sorted.
🔇 Additional comments (28)
packages/cache-handler/src/helpers/is-tag-implicit.ts (1)
1-29: Implementation looks solid and well-documented.This is a clean, focused implementation of a utility function that determines whether a tag is an implicit Next.js-generated tag. The thorough JSDoc documentation clearly explains the purpose, behavior, and provides helpful examples.
The function follows the single responsibility principle and has a clear, descriptive name that matches its purpose. The implementation is straightforward and correct, checking if the tag starts with the Next.js implicit tag identifier.
The import statement correctly uses the
.jsextension, which is appropriate for ES modules and aligns with Next.js 15 module resolution requirements.docs/cache-handler-docs/src/app/usage/creating-a-custom-handler/page.mdx (2)
21-21: Update Import for Renamed FunctionThe import now correctly references the new helper function name
isTagImplicitfrom@neshca/cache-handler/helpers. This update aligns with the renaming changes across the codebase. Please verify that all references to this helper are updated in related files.
182-190: Confirm Consistent Usage ofisTagImplicitThe function call in the
revalidateTagmethod has been updated to useisTagImplicit(tag)instead of the old name. This change appears to be correctly implemented, ensuring consistency with the helper’s new naming. Consider a quick review of the end-to-end cache revalidation flow to ensure that the behavior remains intact.packages/cache-handler/tsconfig.json (3)
2-3: JSON Schema and Extends Update.
The addition of the$schemaproperty ("https://json.schemastore.org/tsconfig") enhances IDE support by enabling schema validation and autocompletion. Updating the"extends"property to"@repo/typescript-config/lib.json"aligns the configuration with the new TypeScript settings for Node.js 20.
5-6: Compiler Options: outDir and rootDir Configuration.
Defining"outDir": "dist"and"rootDir": "src"clearly delineates the source files and the build output folder. This setup promotes consistency in the project’s build process. Ensure that the physical directory structure in the package reflects these settings.
8-9: Include and Exclude Paths Improvement.
Switching the"include"pattern to["**/*.ts"]ensures only TypeScript files are compiled, and updating the"exclude"array to omit"node_modules","dist","test", and"vitest.config.ts"further refines the compilation scope. Verify that no essential files are unintentionally excluded..vscode/settings.json (2)
8-8: Add explicit TypeScript SDK setting
The new"typescript.tsdk": "node_modules/typescript/lib"entry ensures that VS Code uses the project’s locally installed TypeScript, which can improve consistency across development environments. Please confirm that this path matches your project structure and that all team members are aware of this change.
40-42: Consistent Configuration for TypeScriptReact
The[typescriptreact]block now only sets"editor.defaultFormatter": "biomejs.biome", harmonizing it with the other language settings. This change, along with removed configurations (such as file nesting patterns and Grammarly selectors), aligns the settings with the newer ES module and TypeScript setup. Verify that these removals do not impact your team's workflow if any previously relied on those options.packages/cache-handler/src/helpers/create-redis-timeout-config.ts (3)
1-4: Well-structured imports and type definition.The use of
ReturnType<typeof commandOptions>is a good practice for type safety. It ensures that the return type of the utility function stays in sync with any changes to the Redis package's API.
5-24: Excellent documentation with clear examples.The JSDoc comments are comprehensive, well-structured, and include a helpful usage example that demonstrates how to use the function in practice. This kind of thorough documentation significantly improves developer experience and code maintainability.
25-31:❓ Verification inconclusive
Verify browser compatibility of AbortSignal.timeout API.
The
AbortSignal.timeout()method is relatively recent. If this code needs to run in environments that might not support this API, consider checking compatibility or providing a fallback.
🌐 Web query:
What is the browser compatibility for AbortSignal.timeout() and when was it introduced?💡 Result:
The
AbortSignal.timeout()method, introduced to simplify timeout handling in asynchronous operations like fetch requests, has varying browser support:Browser Compatibility
Browser Supported Versions Notes Chrome 103+ (Desktop/Android) Introduced in June 2022 [3] Edge 103+ Chromium-based versions [1] Firefox 100+ Supported since ~May 2022 [1] Safari 16.0+ (macOS/iOS) Requires macOS Ventura/iOS 16 [1][3] Opera 89+ Chromium-based versions [1] Samsung Internet 20+ [1] Key Details
- Unsupported browsers: Internet Explorer (all versions) and pre-2022 browsers like Chrome ≤102, Firefox ≤99, and Safari ≤15.6[1][7].
- Mobile support: Chrome for Android (129+), Firefox for Android (130+), and Safari iOS (16.0+)[1].
- Error handling: Throws
TimeoutErrorDOMException on timeout, distinct from user-triggeredAbortError[4][8].The method was first implemented in Chrome 103 (June 2022)[3], followed by Firefox 100 and Safari 16.0 later that year. It simplifies timeout logic by replacing manual
AbortController+setTimeoutsetups with a single static method[2][5].Citations:
- 1: https://caniuse.com/mdn-api_abortsignal_timeout_static
- 2: https://betterstack.com/community/guides/scaling-nodejs/understanding-abortcontroller/
- 3: https://developer.chrome.com/blog/new-in-chrome-103
- 4: https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/timeout_static
- 5: https://dev.to/rashidshamloo/adding-timeout-and-multiple-abort-signals-to-fetch-typescriptreact-33bb
- 6: https://www.whitespectre.com/ideas/abortcontroller-api-javascript/
- 7: https://www.lambdatest.com/web-technologies/abortcontroller
- 8: whatwg/dom#1082
- 9: https://github.com/shaseley/abort-signal-any/blob/main/signal-memory-management.md
- 10: https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal
AbortSignal.timeout API Usage — Confirm Compatibility and Fallback Strategy
Based on recent browser data,
AbortSignal.timeout()is supported in modern browsers—Chrome 103+, Firefox 100+, Safari 16.0+, Opera 89+, and Samsung Internet 20+—with its initial release around mid-2022. If your project targets only these environments (or Node.js versions that support it), the current implementation is acceptable. However, if legacy browser support (e.g., Internet Explorer or older versions of mainstream browsers) is required, please consider adding a fallback approach (such as using anAbortControllerwith a manualsetTimeout).
- Action: Confirm that all environments running this code meet or exceed these compatibility thresholds.
- Fallback Consideration: If legacy support is needed, update the implementation accordingly.
packages/cache-handler/src/helpers/helpers.ts (1)
1-2:✅ Verification successful
Module path update for ES Module compatibility
The module import paths now use explicit
.jsextensions, which is required for ES Module compatibility in Node.js. This change aligns with the PR objective of adding Next.js 15 support, which relies on ES Modules.Note that function naming has also changed from
isImplicitTagtoisTagImplicitandgetTimeoutRedisCommandOptionstocreateRedisTimeoutConfig, indicating a refactoring of these utilities.
🏁 Script executed:
#!/bin/bash # Verify that the renamed functions are being used consistently across the codebase echo "Checking for any remaining references to old function names..." rg "isImplicitTag|getTimeoutRedisCommandOptions" --type ts echo "Checking usage of new function names..." rg "isTagImplicit|createRedisTimeoutConfig" --type tsLength of output: 5253
Module path and function renaming verified
The explicit
.jsextensions in the import statements and the renaming ofisImplicitTagtoisTagImplicitandgetTimeoutRedisCommandOptionstocreateRedisTimeoutConfighave been successfully propagated across the codebase. The shell script verification confirmed that no legacy names remain and that the changes are consistently applied (e.g., in files underpackages/cache-handler/src/handlers/).packages/cache-handler/src/utils/get-tags-from-headers.ts (1)
1-22: Well-documented utility function with good examplesThe import and JSDoc comments are well-structured with clear descriptions and useful examples demonstrating both string and array header formats.
packages/cache-handler/src/utils/compose-age-estimation-fn.ts (2)
4-18: Simple utility function with clear purposeThe
getInitialExpireAgefunction has a clear purpose and good documentation explaining the rationale behind multiplying by 1.5.
20-60: Well-designed composition function with thorough validationThe
composeAgeEstimationFnfunction is well-structured with:
- Good default implementation
- Thorough validation of returned values
- Proper error handling with descriptive messages
- Clear JSDoc with examples
The code correctly handles edge cases by:
- Flooring floating point numbers
- Capping at MAX_INT32
- Validating positive integers
- Throwing informative assertion errors
This is a robust implementation that follows best practices.
packages/cache-handler/test/utils/compose-age-estimation-fn.test.ts (2)
1-69: Comprehensive test suite for valid inputsThe tests thoroughly cover a wide range of valid inputs and edge cases:
- Simple callbacks
- Floating point handling
- MAX_INT32 boundary handling
- Default callback behavior
- Large values close to MAX_INT32
The tests are well-organized and include descriptive assertions that clearly document the expected behavior.
🧰 Tools
🪛 GitHub Actions: Tests
[error] 3-3: Formatter would have printed the following content: import { composeAgeEstimationFn, getInitialExpireAge } from '../../src/utils/compose-age-estimation-fn.js';
71-120: Thorough testing of invalid inputs and error handlingThe test suite properly covers various error conditions:
- Negative values
- Non-integers
- Zero
- Non-numeric inputs
- Error propagation
- Infinity handling
This ensures the function will behave predictably and provide clear error messages when misused.
packages/cache-handler/src/handlers/local-lru.ts (3)
46-50: Nicely implemented handoff to createConfiguredCache.
The delegation tocreateConfiguredCacheis clean and emphasizes single-responsibility. No concerns here.
57-74: Option definitions look comprehensive.
TheLruCacheOptionsadequately covers typical usage scenarios, and the documentation is clear.
84-105: The createConfiguredCache function is well-structured.
The generic extends an object or string, and the inline usage ofsizeCalculationproperty is appropriate.packages/cache-handler/src/cache-handler.ts (8)
15-16: Use ofCachedRouteKindenum is appropriate.
Adopting the enumerated type fromnext-common-types.jsinstead of string literals is a solid improvement for type safety and maintainability.
18-19: Confirm ES module import extension behavior.
Importing.jsfiles in a.tsfile may require specific TypeScript configurations (e.g."moduleResolution": "NodeNext"). Please ensure your build setup supports ESM imports with explicit.jsextensions.Would you like me to provide a script to grep for your tsconfig entries to confirm ESM readiness?
442-442: Refactor from string literal to enum constant.
Replacing'PAGE'withCachedRouteKind.PAGESaligns the code with the enumerated types and reduces magic strings.
626-626: Verify equivalent functionality of new age estimation function.
Switching tocomposeAgeEstimationFnmay change how expiration ages are calculated. Ensure no unintended side effects or discrepancies compared to the previous approach.If you prefer, I can help devise tests to confirm parity with the old function’s logic.
896-902: Apply tags only toPAGESandAPP_PAGEcases.
UsinggetTagsFromHeadersconditionally forCachedRouteKind.PAGESorAPP_PAGEis consistent with enumerated usage. No obvious issues.
909-909: Storing the incremental cache value is correct.
Saving theincrementalCacheValuedirectly into the.valueproperty is straightforward and maintains clarity.
914-917: Confirm fallback false handling forAPP_PAGE.
ForPAGES, fallback false triggers file system writes. If you intend the same logic forAPP_PAGE, consider adding an additional check (or combine the conditions). If it’s intentional to exclude, disregard.
831-831:❓ Verification inconclusive
Mandatory
ctxparameter may cause breaking changes.
Previously, callers could omit the second parameter. Now thatctxis mandatory, confirm all call sites pass it in.Here’s a script to search for
.get(calls that might be missing thectxargument:
🏁 Script executed:
#!/bin/bash rg -A 2 "CacheHandler\.get\("Length of output: 31
ACTION: MANUALLY VERIFY ALL
CacheHandler.getCALLS INCLUDE THE NEWctxARGUMENTThe automated search using the previous rg command (and our follow‐up with a PCRE2 regex) did not return any instances where
CacheHandler.getis invoked with only one argument. However, because this result may be affected by formatting variations or other nuances in how the method is called, it’s best to double-check manually that no call site misses providing the mandatoryctxparameter.
- Location of Change:
packages/cache-handler/src/cache-handler.ts(line 831) now requires a non-optionalctxparameter.- Action: Confirm throughout the repository that every invocation of
CacheHandler.getexplicitly passes thectxparameter.Please verify this manually to ensure no regressions occur due to this breaking change.
| import { MAX_INT32 } from '../../src/constants.js'; | ||
| import { composeAgeEstimationFn, getInitialExpireAge } from '../../src/utils/compose-age-estimation-fn.js'; |
There was a problem hiding this comment.
Fix import formatting according to pipeline failure
There's a formatting issue with the import statement reported by the pipeline.
Fix the import formatting according to your style guidelines:
import { describe, expect, it } from 'vitest';
import { MAX_INT32 } from '../../src/constants.js';
-import { composeAgeEstimationFn, getInitialExpireAge } from '../../src/utils/compose-age-estimation-fn.js';
+import {
+ composeAgeEstimationFn,
+ getInitialExpireAge,
+} from '../../src/utils/compose-age-estimation-fn.js';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { MAX_INT32 } from '../../src/constants.js'; | |
| import { composeAgeEstimationFn, getInitialExpireAge } from '../../src/utils/compose-age-estimation-fn.js'; | |
| import { MAX_INT32 } from '../../src/constants.js'; | |
| import { | |
| composeAgeEstimationFn, | |
| getInitialExpireAge, | |
| } from '../../src/utils/compose-age-estimation-fn.js'; |
🧰 Tools
🪛 GitHub Actions: Tests
[error] 3-3: Formatter would have printed the following content: import { composeAgeEstimationFn, getInitialExpireAge } from '../../src/utils/compose-age-estimation-fn.js';
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/cache-handler/src/handlers/local-lru.ts (1)
134-136: 💡 Verification agent❓ Verification inconclusive
Verify integration with existing handler
The existing
createHandlerfunction now uses the updatedcreateCacheStorefunction. Since this function signature has been preserved, the integration should work seamlessly. However, I recommend adding a test to verify that the cache behaves as expected with the new size calculation implementation.
🏁 Script executed:
#!/bin/bash # Check if there are any tests that verify the LRU cache's behavior fd -e test.ts -e spec.ts -e test.js -e spec.js -p "packages/cache-handler" | xargs grep -l "local-lru" | sortLength of output: 107
Action Required: Add Tests for LRU Cache Behavior
The integration of
createCacheStorewithcreateHandlerremains intact since the function signature is preserved. However, our search did not locate any test files referencing thelocal-lruhandler in thepackages/cache-handlerdirectory. To ensure that the new size calculation implementation behaves as expected, please add integration tests covering the LRU cache functionality.
🧹 Nitpick comments (15)
packages/cache-handler/src/instrumentation/register-initial-cache.ts (1)
7-7: Consider a direct type import for simpler maintainability.
IfCacheHandleris purely a type, you can directly import it by name usingimport type { CacheHandler }to eliminate the dynamictypeof import()approach. This can help with readability and reduce complexity if the module is statically referenced.apps/cache-testing/cache-handler-none.js (2)
1-19: Consider adding documentation to explain purpose and usage of this no-op cache handler.The implementation of a "no-op" cache handler is clean and follows the expected interface pattern, with all methods properly returning resolved promises with
undefined. This is valuable for testing scenarios where you want to disable caching completely.To improve maintainability:
- Add JSDoc comments explaining the purpose of this handler
- Document when this handler should be used versus other caching implementations
// @ts-check import { CacheHandler } from '@neshca/cache-handler'; +/** + * A no-op cache handler that doesn't perform any caching operations. + * Useful for testing scenarios or environments where caching should be disabled. + */ CacheHandler.onCreation(() => { return { handlers: [ { name: 'handler-none', + /** + * No-op get implementation + * @param {any} key - The cache key + * @returns {Promise<undefined>} Always returns undefined + */ get: () => Promise.resolve(undefined), set: () => Promise.resolve(undefined), revalidateTag: () => Promise.resolve(undefined), delete: () => Promise.resolve(undefined), }, ], }; }); export default CacheHandler;
8-14: Consider implementing parameter types for better type safety.While the file uses
@ts-check, adding explicit parameter types to the handler methods would improve type safety and make the expected interface clearer.{ name: 'handler-none', - get: () => Promise.resolve(undefined), - set: () => Promise.resolve(undefined), - revalidateTag: () => Promise.resolve(undefined), - delete: () => Promise.resolve(undefined), + get: (key) => Promise.resolve(undefined), + set: (key, value, options) => Promise.resolve(undefined), + revalidateTag: (tag) => Promise.resolve(undefined), + delete: (key) => Promise.resolve(undefined), },packages/cache-handler/src/cache-handler.ts (1)
883-883: Simplified parameter destructuringThe destructuring syntax is more concise, improving code readability by clearly showing which properties are being extracted from the context object.
packages/cache-handler/src/handlers/local-lru.ts (1)
148-153: Consider using the spread operator directly on the Set constructorThe code creates a combined set of tags by spreading arrays into a new Set. While this works correctly, you could simplify it by passing both arrays directly to the Set constructor.
- const combinedTags = new Set([ - ...cacheValue.tags, - ...sanitizedImplicitTags, - ]); + const combinedTags = new Set([...cacheValue.tags, ...sanitizedImplicitTags]);Alternatively, for even better performance with large arrays:
- const combinedTags = new Set([ - ...cacheValue.tags, - ...sanitizedImplicitTags, - ]); + const combinedTags = new Set(cacheValue.tags); + sanitizedImplicitTags.forEach(tag => combinedTags.add(tag));packages/cache-handler/src/use-cache/redis.ts (6)
15-44: Consider supporting additional authentication and SSL options in RedisConfig.While this configuration covers the basics, some deployments may require username/password and SSL/TLS configuration. Extending the
RedisConfigtype to include these fields (or documenting how users can pass them viaclientOptions) can improve flexibility for secure and production use cases.
63-75: Add optional metadata to tag invalidation messages for debugging.Currently, the
TagInvalidationMessagetype includes basic fields. Adding optional context (e.g., the invalidation source or environment information) can simplify root-cause analysis in distributed caching environments.
202-234: Handle large tag sets or partial loading to avoid potential memory issues.Loading all tags into
localTagsManifest.itemscan lead to high memory usage if there are thousands of tags. Consider strategies like partitioning or partial loading from Redis, or pruning stale tags more aggressively, to keep memory consumption managed.
358-448: Avoid storing very large streams entirely in memory before base64 encoding.Reading the entire stream into a buffer may be fine for smaller objects, but can degrade performance when dealing with large payloads. Consider streaming directly to Redis, chunk-by-chunk, or setting size limits on cacheable items to minimize memory usage.
455-510: Consider concurrency and partial invalidation strategies for tagged data.When multiple clients attempt to invalidate overlapping tags, race conditions could arise if new tags are being added at the same time. Evaluating concurrency controls (e.g., Lua scripts or version checks) may keep data consistent in high-traffic scenarios.
527-530: Use caution with the default export instantiating a global Redis client.Returning a default Redis handler instance can introduce unexpected side effects if multiple imports configure or reuse it differently. Encourage consumers to use
createRedisCacheHandlerdirectly if they need separate connections or custom configurations.apps/cache-testing/next.config.ts (1)
12-12: Consider removing or explaining commented-out code.
If you no longer needoutputFileTracingRoot, removing or clarifying this line will reduce clutter.- // outputFileTracingRoot: path.join(import.meta.dirname, '../../'),apps/cache-testing/src/utils/create-get-data.ts (2)
24-24: Suggestion: Add additional error handling to fetch.
Wrapping the fetch call with try/catch can offer more robust error diagnosis.- const result = await fetch(url); + let result; + try { + result = await fetch(url); + } catch (err) { + console.error('Fetch failed:', err); + return null; + }
26-27: Minimal error checks.
Returning null is fine, but consider logging error details for debugging.apps/cache-testing/src/app/app/no-params/dynamic-true/[slug]/page.tsx (1)
8-8: Hardcoded revalidate.
Consider reading from config or environment variables for flexibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
apps/cache-testing/cache-handler-none.js(1 hunks)apps/cache-testing/next.config.ts(1 hunks)apps/cache-testing/redis.js(1 hunks)apps/cache-testing/src/app/app/no-params/dynamic-false/[slug]/page.tsx(1 hunks)apps/cache-testing/src/app/app/no-params/dynamic-true/[slug]/page.tsx(1 hunks)apps/cache-testing/src/app/app/no-params/ssr/200/page.tsx(1 hunks)apps/cache-testing/src/app/app/randomHex/[length]/page.tsx(1 hunks)apps/cache-testing/src/app/app/with-params/dynamic-false/[slug]/page.tsx(2 hunks)apps/cache-testing/src/app/app/with-params/dynamic-true/[slug]/page.tsx(2 hunks)apps/cache-testing/src/utils/create-get-data.ts(1 hunks)apps/cache-testing/tests/app.spec.ts(1 hunks)apps/cache-testing/tests/pages.spec.ts(1 hunks)packages/cache-handler/package.json(2 hunks)packages/cache-handler/src/cache-handler.ts(8 hunks)packages/cache-handler/src/handlers/experimental-redis-cluster.ts(9 hunks)packages/cache-handler/src/handlers/local-lru.ts(1 hunks)packages/cache-handler/src/instrumentation/register-initial-cache.ts(3 hunks)packages/cache-handler/src/next-common-types.ts(3 hunks)packages/cache-handler/src/use-cache/redis.ts(1 hunks)packages/cache-handler/test/utils/compose-age-estimation-fn.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/cache-testing/redis.js
🚧 Files skipped from review as they are similar to previous changes (9)
- apps/cache-testing/tests/pages.spec.ts
- packages/cache-handler/src/handlers/experimental-redis-cluster.ts
- apps/cache-testing/src/app/app/with-params/dynamic-true/[slug]/page.tsx
- packages/cache-handler/src/next-common-types.ts
- apps/cache-testing/src/app/app/randomHex/[length]/page.tsx
- packages/cache-handler/test/utils/compose-age-estimation-fn.test.ts
- apps/cache-testing/src/app/app/with-params/dynamic-false/[slug]/page.tsx
- apps/cache-testing/src/app/app/no-params/dynamic-false/[slug]/page.tsx
- packages/cache-handler/package.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: redis-strings
- GitHub Check: redis-stack
🔇 Additional comments (37)
apps/cache-testing/tests/app.spec.ts (1)
144-144:✅ Verification successful
The test description change correctly reflects the modified Next.js 15 caching behavior.
The updated test description now states the page should be fresh after "becoming stale and reloaded" instead of "becoming stale and reloaded twice". This aligns with the Next.js 15 caching model, which now requires only a single reload to observe fresh data after a cache entry becomes stale, rather than multiple reloads as in previous versions.
The test implementation appears consistent with this behavior change and follows good testing practices by validating the expected cache refresh pattern.
🏁 Script executed:
#!/bin/bash # Check if Next.js 15 is mentioned in package.json or other configuration files echo "Looking for Next.js version references:" grep -r --include="*.json" --include="*.md" "next\":" . | head -5Length of output: 416
Test description update for Next.js 15 caching behavior verified
The test now reads:
test(`Page should be fresh after becoming stale and reloaded ${path}`, async ({which correctly reflects that a single reload is sufficient for observing fresh data after staleness.
Verification confirms that the project uses Next.js version 15.2.0 in multiple packages (e.g., in
apps/cache-testing/package.json), aligning with the behavior change described.The updated test description and implementation are consistent with the new caching model.
apps/cache-testing/src/app/app/no-params/ssr/200/page.tsx (2)
8-8: Return type broadened appropriatelyChanging from
Promise<JSX.Element>toPromise<React.ReactNode>is a good practice that provides more flexibility in what the component can return. This aligns with Next.js 15's typing conventions and is consistent with other component updates in the project.
6-6:❓ Verification inconclusive
Verify caching behavior change
The parameter in
createGetDatahas been changed from using'no-store'to0, which likely alters the caching behavior of this component. Make sure this change is intentional and aligns with Next.js 15's caching expectations.
🏁 Script executed:
#!/bin/bash # Check implementation of createGetData to understand parameter impact echo "Searching for createGetData implementation..." fd -t f "create-get-data" --exec cat {}Length of output: 1083
Caching Behavior Verification: Confirm Revalidate Value
It appears that the caching parameter passed to
createGetDatahas been updated from using'no-store'to0. In the implementation, this0value is directly passed as therevalidateparameter to Next.js’sunstable_cacheLife. Please confirm that settingrevalidateto0aligns with Next.js 15’s caching expectations and achieves the intended caching strategy. Verify that this change does not inadvertently alter cache invalidation or data freshness compared to the previous behavior.packages/cache-handler/src/instrumentation/register-initial-cache.ts (3)
154-159: Parallel file reading looks good—just ensure robust handling of JSON parse errors.
Reading.htmland.jsonin parallel withPromise.allis efficient. The fallback error handling is in place, so this approach should work well as long as the downstream logic anticipates possible missing or malformed JSON.
177-181: Confirm consumers are aware of omitted headers and status.
You're now settingheadersandstatustoundefinedin the cache. Ensure that any consumers expecting headers or a status code can handle the absence of this data without raising errors. Consider adding a fallback or documenting this behavior in the public API.
3-5:❓ Verification inconclusive
Confirm stability of internal Next.js imports.
Imports ofnext/constants.jsandnext/dist/build/index.jsrely on internal Next.js structures and may break with future releases if Next.js significantly refactors its internals. Consider verifying official guidance or adjusting to documented public exports to ensure compatibility with Next.js 15 and beyond.Would you like to run a quick search to confirm Next.js 15 includes stable references for these modules?
🏁 Script executed:
#!/bin/bash # Verify that next/constants.js and next/dist/build/index.js exist fd --glob 'node_modules/next/**/constants.js' fd --glob 'node_modules/next/**/build/index.js'Length of output: 97
Attention: Manual Verification Required for Next.js Internal Imports
The initial verification script did not yield output confirming the existence or stability of
next/constants.jsandnext/dist/build/index.js. Since these imports depend on internal Next.js structures that may change in future releases, please manually verify the following:
- Confirm that these paths exist in your current Next.js version.
- Check the official Next.js documentation or release notes for guidance on using internal APIs.
- Consider switching to documented public exports to ensure future compatibility (especially for Next.js 15 and onward).
apps/cache-testing/cache-handler-none.js (1)
19-19:❓ Verification inconclusive
Ensure this cache handler is properly tested.
Since this is a new file implementing a specific caching strategy (or lack thereof), verify that tests exist to confirm its behavior works as expected when integrated with Next.js.
🏁 Script executed:
#!/bin/bash # Check if there are tests for the none cache handler echo "Searching for tests related to the none cache handler..." rg -i "handler-none|cache-handler-none" --type js --type ts --type jsx --type tsxLength of output: 234
Action: Verify None Cache Handler Testing
It appears our initial test search for "cache-handler-none" encountered an error with the
jsxfile type flag. Please manually verify that tests exist for this new caching strategy and that they correctly cover its integration with Next.js. In particular, confirm that:
- Tests cover the expected behavior (or lack thereof) of the cache handler.
- Integration tests validate its functionality within the Next.js environment.
If tests are missing or insufficient, consider adding them to ensure comprehensive coverage.
packages/cache-handler/src/cache-handler.ts (8)
14-15: Import paths updated to use .js extension for ES modules compatibilityThe import paths now include explicit
.jsextensions, which is required for proper ES module resolution in Next.js 15. This change aligns with the ECMAScript modules specification.
440-440: Improved type safety using enumeration instead of string literalReplacing the string literal
'PAGE'with the enum valueCachedRouteKind.PAGESimproves type safety and maintainability by preventing typos and providing better IDE support.
831-833: Added type-safe check for page router kindThe new check for
ctx?.kind !== IncrementalCacheKind.PAGESearly returns for non-Pages router content. This improves the method's clarity by explicitly handling only supported router types.
868-870: Added type-safe check for cache value kindSimilar to the get method, the set method now explicitly checks for
incrementalCacheValue?.kind !== CachedRouteKind.PAGESand early returns for unsupported kinds. This improves type safety and makes the behavior more predictable.
898-903: Refactored cache value creation for improved clarityThe restructured cache value definition is more concise and clear. Using
as constalso provides better type safety by ensuring the object's properties are read-only.
915-917: Simplified revalidateTag implementationThe
revalidateTagmethod has been simplified to return an empty promise with a comment indicating it's not implemented yet. This is acceptable for a work-in-progress PR, but should be properly implemented before final release.Is this intentional as part of the Next.js 15 migration? Consider whether this functionality needs to be implemented before the migration is complete, as it may affect applications relying on tag-based revalidation.
17-17:❓ Verification inconclusive
Function replacement for age estimation
The
composeAgeEstimationFnutility has been introduced to replace the previous age estimation implementation. This appears to be part of the ES modules update.
🏁 Script executed:
#!/bin/bash # Look for the implementation of composeAgeEstimationFn to verify its behavior matches the previous function fd "compose-age-estimation-fn.js" --type f --exec cat {} \;Length of output: 62
Action Required: Verify the presence of the replacement utility implementation
The import update in
packages/cache-handler/src/cache-handler.tsnow points to:import { composeAgeEstimationFn } from './utils/compose-age-estimation-fn.js';Our automated search did not locate a file named
compose-age-estimation-fn.jsanywhere in the repository. Please verify manually that:
- The file exists at the expected location (
packages/cache-handler/src/utils/compose-age-estimation-fn.js).- It contains the intended replacement logic for the previous age estimation implementation.
Once confirmed, we can confidently approve the change.
624-624:❓ Verification inconclusive
Updated age estimation function implementation
The previous
createValidatedAgeEstimationFunctionhas been replaced withcomposeAgeEstimationFnfrom a new utility file. Make sure the new implementation maintains the same behavior or improves upon it.
🏁 Script executed:
#!/bin/bash # Compare the old and new implementation of the age estimation functions rg -A 10 "createValidatedAgeEstimationFunction" --type tsLength of output: 57
Updated age estimation function implementation replacement
The oldcreateValidatedAgeEstimationFunctionis no longer present in the codebase and has been fully replaced bycomposeAgeEstimationFnas seen on line 624. Although the absence of references suggests the transition is complete, please manually verify that:
- Behavior Consistency: The new utility function retains the age estimation logic (or improves upon it) compared to the old implementation.
- Test Coverage: Existing tests for cache expiration behave as expected with the new function.
packages/cache-handler/src/handlers/local-lru.ts (6)
1-7: Import statements updated for ES module compatibilityThe import statements have been properly updated to use ES module syntax with explicit
.jsextensions. This change aligns with the project's migration from CommonJS to ES modules as mentioned in the PR summary.
9-44: Well-structured size calculation function with comprehensive case handlingThe
calculateObjectSizefunction provides a systematic approach to calculating cache entry sizes based on their kind. The implementation handles eachCachedRouteKindcase appropriately:
- Provides a default size (25) for falsy values
- Calculates string length for redirects and fetches
- Handles special cases for pages and app routes
- Explicitly prevents image caching through error throwing
- Falls back to zero size for unhandled cases
This granular approach to size calculation will help manage memory usage more effectively.
46-50: Simplified cache store creation with better compositionThe
createCacheStorefunction has been refactored to leverage the newcreateConfiguredCacheutility function, which improves code organization and reusability. This separation of concerns allows for easier maintenance going forward.
52-74: Well-documented type definition with appropriate defaultsThe
LruCacheOptionstype has comprehensive JSDoc comments that clearly document the purpose, defaults, and version information for each option. This level of documentation significantly improves developer experience and code maintainability.
76-82: Constants for default cache configurationExtracting the cache configuration values into named constants improves code readability and maintainability. The values (1000 items and 100MB max size) seem reasonable defaults for a local LRU cache.
84-105: Generic cache configuration utility with flexible size calculationThe
createConfiguredCachefunction is a well-designed utility that:
- Accepts a generic type parameter for the cache value type
- Takes a callback function for size calculation
- Uses parameter destructuring with defaults for clean option handling
- Properly configures the LRUCache with the provided options
This implementation provides flexibility while maintaining type safety, making it easier to create different types of caches with custom size calculations.
packages/cache-handler/src/use-cache/redis.ts (1)
291-356:✅ Verification successful
Verify usage of
performance.timeOrigin + performance.now()in Node.js.Some Node.js versions may not fully support the global
performanceAPI in the same way as browsers do. This might cause unexpected results or undefined behavior in older environments.
🌐 Web query:
Is the global performance API, specifically timeOrigin and now(), reliably supported in Node.js 18+ and Next.js 15?💡 Result:
The global Performance API, including
timeOriginandnow(), is reliably supported in Node.js 18+ and Next.js 15, with some nuances to consider:
Node.js 18+ Support
performance.timeOrigin
- Added in Node.js 16.0.0 and stable in later versions, including Node.js 18+ [9][10].
- Returns a high-resolution timestamp representing the start of the current process, similar to browser behavior.
performance.now()
- Part of the
perf_hooksmodule, available since Node.js 8.5.0.- In Node.js 18+, it uses a monotonic clock (not affected by system time changes) for reliability [2][9].
Global Scope
- Unlike browsers, Node.js requires importing
perf_hooksfor full access:const { performance } = require('perf_hooks'); // or in ESM: import { performance } from 'node:perf_hooks';- Some methods (e.g.,
timeOrigin) are directly available globally in newer versions [9].
Next.js 15 Support
Next.js 15 inherits Node.js 18+ compatibility but has specific considerations:
App Router vs. Edge Runtime
- In standard App Router (Node.js runtime),
performance.timeOriginandnow()work as expected.- In Edge Runtime (e.g., middleware), Node.js APIs like
perf_hooksare not supported [8]. Use browser-compatible APIs instead.Metrics in Server Components
Next.js 15 introducesunstable_afterfor executing code post-response, which can be used withperformance.now():import { unstable_after as after } from 'next/server'; export default function Page() { const start = performance.now(); // ... logic after(() => { const duration = performance.now() - start; console.log(`Task took ${duration}ms`); }); return <div>Content</div>; }This avoids blocking responses while capturing performance data [4][7].
Key Considerations
Feature Node.js 18+ Next.js 15 (Node) Next.js 15 (Edge) performance.now()✅ Yes ✅ Yes ❌ No performance.timeOrigin✅ Yes ✅ Yes ❌ No Monotonic Clock ✅ Yes ✅ Yes ❌ No For Edge Runtime, use the Web Performance API (
window.performancein browsers) or platform-specific alternatives likeDate.now()[8][10].
Summary: Node.js 18+ and Next.js 15 fully support
performance.timeOriginandnow()in Node.js environments, but Edge Runtime in Next.js restricts access to Node.js-specific APIs. Always verify the runtime context when using performance metrics.Citations:
- 1: https://www.nearform.com/insights/whats-new-in-node-js-18/
- 2: https://developer.mozilla.org/en-US/docs/Web/API/Performance/timeOrigin
- 3: https://nextjs.org/blog/next-15
- 4: https://docs.getunleash.io/reference/sdks/next-js
- 5: https://sdtimes.com/softwaredev/node-js-18-available-with-fetch-api-enabled-by-default/
- 6: https://forum.serverless.com/t/ci-cd-issue-for-node-18-x-runtime/18211
- 7: https://nextjs.org/blog/next-15-rc2
- 8: https://answers.netlify.com/t/404-on-all-pages-of-a-next-js-application-with-app-router-setup/93605
- 9: https://nodejs.org/en/blog/announcements/v18-release-announce
- 10: parcel-bundler/parcel#9530
Confirmed: Global
performanceAPI usage is reliable in our Node.js environment.
- The web query confirms that both
performance.timeOriginandperformance.now()are fully supported in Node.js 18+ and in Next.js 15 when running in the standard Node environment.- Since the cache handler is expected to run on a Node server, the current usage is safe.
- Note: If this code were to run in a Next.js Edge Runtime context, alternative approaches would be required. However, as it stands, no changes are needed.
apps/cache-testing/next.config.ts (4)
2-2: Use of NextConfig import.
No issues; this addition strengthens type safety.
6-6: Typed configuration is a good practice.
Definingconst nextConfig: NextConfigincreases clarity and maintainability.
18-21:✅ Verification successful
Ensure path consistency for caching logic.
Double-check that'./redis.js'is correct and not meant to referencecache-handler-redis-stack.js.
🏁 Script executed:
#!/bin/bash # Searching for redis.js in the repo fd "redis.js"Length of output: 41
Verified: The caching handler path is correct.
The fileredis.jsexists inapps/cache-testingas expected, so referencing'./redis.js'innext.config.tsis valid. No changes are needed.
4-4:✅ Verification successful
Verify that the specified file path exists.
Ifcache-handler-redis-stack.jsis not guaranteed to exist, please confirm and adjust accordingly.
🏁 Script executed:
#!/bin/bash # Searching for cache-handler-redis-stack.js in the repo fd "cache-handler-redis-stack.js"Length of output: 81
Verified file path exists:
The filecache-handler-redis-stack.jsis confirmed to exist atapps/cache-testing/cache-handler-redis-stack.js. Since this path is correct relative toapps/cache-testing/next.config.ts, no changes are necessary.apps/cache-testing/src/utils/create-get-data.ts (8)
8-8: Simplified function signature looks good.
Removing cache strategy parameters reduces complexity and centralizes caching logic.
12-12: 'use cache' directive.
This directive is typically for Next.js server-side caching. Confirm it matches your intended caching behavior.
14-14: No immediate concerns.
ConstructingpathAndTagwithnormalizeSlugis clear and straightforward.
16-18: Cache revalidation approach approved.
UsingcacheLifewith a dynamicrevalidateis a solid pattern for precise control.
20-20: Tagging with cacheTag.
Straightforward usage; no issues identified.
22-22: Constructing local fetch URL.
No concerns; the approach is conventional for local service calls.
30-30: JSON parsing successfully integrated.
No issues; looks correct.
1-4:❓ Verification inconclusive
Confirm the stability of 'unstable_cacheLife' & 'unstable_cacheTag'.
Their naming suggests experimental APIs. Please verify they are safe for your production environment.
🌐 Web query:
Are next/cache's unstable_cacheLife and unstable_cacheTag APIs stable in Next.js 15?💡 Result:
Next.js 15 introduces significant caching improvements, but the
unstable_cacheLifeandunstable_cacheTagAPIs remain experimental and are not yet stable. Here's a breakdown:Key Points
Experimental Status
- Both APIs are marked as unstable and require enabling the
dynamicIOexperimental flag innext.config.js[4][10].- They are currently available in the canary channel and subject to change[4][10].
Replacement for
unstable_cache
- The
unstable_cacheAPI (deprecated in v15) is being phased out in favor of theuse cachedirective, which integrates withcacheLifeandcacheTag[1][4].- Example using the new pattern:
'use cache' import { unstable_cacheLife as cacheLife } from 'next/cache' export default async function Page() { cacheLife('hours') // Configures cache lifetime return <div>Page</div> }Caching Configuration
cacheLifeallows setting predefined or custom cache profiles (e.g.,seconds,days) for granular control[4].cacheTagenables cache invalidation viarevalidateTag[4].Current Limitations
- The
dynamicIOflag (required for these APIs) is only available in canary builds[10].- Stability timelines are unclear, with ongoing community feedback shaping the APIs[3][10].
Recommendations
- For production, continue using
unstable_cacheuntiluse cachestabilizes.- Monitor Next.js canary releases and official documentation for updates[4][10].
In summary, while these APIs offer powerful caching customization, they are not yet stable and should be used cautiously in non-canary environments.
Citations:
- 1: https://nextjs.org/docs/app/api-reference/functions/unstable_cache
- 2: vercel/next.js#54075
- 3: https://hackernoon.com/nextjs-v15-reflecting-on-previous-mistakes
- 4: https://nextjs.org/docs/app/api-reference/functions/cacheLife
- 5: https://nextjs.org/blog/next-15
- 6: https://nextjs.org/docs/app/building-your-application/caching
- 7: vercel/next.js#75462
- 8: https://www.youtube.com/watch?v=-FMwdg2wx4M
- 9: https://blog.logrocket.com/caching-next-js-unstable-cache/
- 10: vercel/next.js#72101
Caution on Using Experimental Caching APIs
We've confirmed that both
unstable_cacheLifeandunstable_cacheTagremain experimental in Next.js 15. These APIs require enabling thedynamicIOexperimental flag and are only available on canary builds, so they are not recommended for production unless you thoroughly validate their behavior. Consider evaluating stable alternatives—such as continuing with the deprecatedunstable_cacheor migrating to the newuse cachedirective—until these APIs are officially stabilized.apps/cache-testing/src/app/app/no-params/dynamic-true/[slug]/page.tsx (3)
10-10: Promised params approach.
Be certain this pattern is valid in your Next.js version; dynamic route params are often plain objects, not promises.
12-12: Correct usage of createGetData.
Passingrevalidateto unify caching logic is consistent with your design.
16-18: Asynchronous parameter resolution.
Awaiting the promise forparamsworks, though it's an unusual pattern. Confirm your route definitions align with this structure.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/cache-handler/src/instrumentation/register-initial-cache.ts (1)
29-63: 🛠️ Refactor suggestionFunction documentation is outdated.
The function documentation still mentions populating the cache with "routes" and "fetch calls" (lines 34-35), along with a
fetchoption (line 41) and aroutesoption (line 45), but according to the implementation these features have been removed.Update the documentation to accurately reflect the current functionality. Here's a suggested change:
/** * Populates the cache with the initial data. * * * By default, it includes the following: * - Pre-rendered pages - * - Routes - * - Fetch calls * * * @param CacheHandler - The configured CacheHandler class, not an instance. * * * @param [options={}] - Options for the instrumentation. See {@link RegisterInitialCacheOptions}. * * - * @param [options.fetch=true] - Whether to populate the cache with fetch calls. * * @param [options.pages=true] - Whether to populate the cache with pre-rendered pages. * - * @param [options.routes=true] - Whether to populate the cache with routes. * * @example file: `instrumentation.ts` * * ```js * export async function register() { * if (process.env.NEXT_RUNTIME === 'nodejs') { * const { registerInitialCache } = await import('@neshca/cache-handler/instrumentation'); * // Assuming that your CacheHandler configuration is in the root of the project and the instrumentation is in the src directory. * // Please adjust the path accordingly. * // CommonJS CacheHandler configuration is also supported. * const CacheHandler = (await import('../cache-handler.mjs')).default; * await registerInitialCache(CacheHandler); * } * } * ``` * * @since 1.7.0 */
🧹 Nitpick comments (5)
packages/cache-handler/src/instrumentation/register-initial-cache.ts (2)
18-27: Documentation needs updating to reflect removed options.The RegisterInitialCacheOptions type has been simplified to only include the
pagesoption, but the function documentation (lines 29-63) still mentionsfetchandroutesoptions which have been removed from the implementation.Please update the function documentation to reflect the current available options.
56-56: The example code should use.jsextension.The example in the documentation shows importing from
'../cache-handler.mjs', but with the migration to ES modules, this should probably use the.jsextension to be consistent with the rest of the codebase.- const CacheHandler = (await import('../cache-handler.mjs')).default; + const CacheHandler = (await import('../cache-handler.js')).default;packages/cache-handler/src/cache-handler.ts (3)
886-886: Minor naming nitpick.
neshca_lastModifiedis a bit unconventional. If it's a shared field, consider using a more general name likelastModifiedIdentifier.
903-908: Consider the usage of empty tags.
Iftagsremain permanently empty, you could make them optional or remove them to reduce clutter. Otherwise, this is fine if you plan to add tags later.
920-921:revalidateTagnot implemented.
Currently a placeholder. Let me know if you’d like assistance implementing this method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/cache-handler/src/cache-handler.ts(8 hunks)packages/cache-handler/src/instrumentation/register-initial-cache.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: redis-strings
- GitHub Check: redis-stack
🔇 Additional comments (14)
packages/cache-handler/src/instrumentation/register-initial-cache.ts (4)
3-5: Import paths updated for Next.js 15 compatibility.The import paths have been updated to include
.jsextensions, which is consistent with ES module syntax required by Next.js 15. The imports have also been reorganized to use the new type structure.
7-7: Updated CacheHandlerType import path for ES modules.The import path for CacheHandlerType has been updated to include the
.jsextension for ES module compatibility.
154-159: Improved Promise.all pattern for file reading.The Promise.all pattern with immediate JSON parsing looks good for concurrent file reading operations.
177-187: Replaced string literals with enum and updated cache structure.Good improvements:
- Replaced hardcoded 'PAGE' string with CachedRouteKind.PAGES enum
- Explicitly set headers and status to undefined
- Updated the cache options structure
These changes make the code more maintainable and type-safe.
packages/cache-handler/src/cache-handler.ts (10)
14-15: Imports approved.
These import statements correctly bring inCachedRouteKindandIncrementalCacheKindfrom the shared types file. No concerns here.
17-17: New utility import looks good.
ImportingcomposeAgeEstimationFnis straightforward and matches usage below.
624-624: Verify behavior whenestimateExpireAgeis undefined.
Ensure thatcomposeAgeEstimationFnhandles a missing or undefined parameter gracefully, preventing runtime errors or unexpected behavior.Would you like me to generate a script that searches for usage of
ttl.estimateExpireAgeto confirm it is never called with invalid inputs?
829-830: [Duplicate] Thectxparameter is now mandatory.
This reaffirms the previous breaking change comment about requiringctx. Ensure that all call sites now supply this parameter without relying on a default.
831-833: Confirm null return for non-PAGES requests.
Returningnullifctx.kindis notIncrementalCacheKind.PAGESmay disregard other valid cache scenarios. Verify this is intentional.
835-835: Configuration call looks good.
Invoking#configureCacheHandler()before proceeding ensures that the handler is properly set up.
848-848: Implicit tags array is acceptable.
Storing an empty array forimplicitTagsis reasonable if you plan to populate it in the future.
869-874: Validate non-caching condition.
Skipping cache whenincrementalCacheValue.kind !== PAGESorctx.fetchCache === truemight exclude some valid use cases. Confirm that conditional is correct.
888-888: Revalidate extraction is straightforward.
ExtractingrevalidatefromcacheControlis clear and maintains readability.
912-912: Good fallback check.
Writing pages to the file system upon fallback expansion appears correct for pages router behavior.
better-salmon
force-pushed
the
upgrade-next-js
branch
from
339d5c0 to
7135e65
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/cache-handler/src/cache-handler.ts (1)
920-921:revalidateTagremains unimplemented.
Would you like assistance drafting an implementation that iterates over handlers or extends the existing revalidation logic?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/cache-testing/tests/pages.spec.ts(1 hunks)packages/cache-handler/src/cache-handler.ts(8 hunks)packages/cache-handler/src/instrumentation/register-initial-cache.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/cache-testing/tests/pages.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: redis-strings
- GitHub Check: redis-stack
🔇 Additional comments (16)
packages/cache-handler/src/instrumentation/register-initial-cache.ts (7)
3-5: Import path updates for ES Module syntax complianceThe import paths have been updated to include file extensions as required by ES modules syntax:
next/constants.jsinstead ofnext/constantsnext/dist/build/index.jsinstead ofnext/dist/build- Changed from
@repo/next-commonto a relative path../next-common-types.jsThese changes align with the NextJS 15 move toward native ES modules.
7-7: Updated import path for CacheHandlerTypeThe type import now includes the
.jsextension for ES module compatibility.
154-159: Improved Promise handling in setPageCacheThe Promise.all implementation has been refined with proper chaining of the JSON parse operation. This is a cleaner approach that properly handles the parsing step.
177-177: Using enum instead of hardcoded stringChanged from hardcoded string 'PAGE' to use the
CachedRouteKind.PAGESenum value. This is a good practice for type safety and maintainability.
180-187: Simplified cache structure and metadata handlingThe cache structure has been simplified:
- Headers and status are now explicitly set to undefined instead of using metadata
- Cache options are now more explicitly structured with
neshca_lastModified,fetchCache, andcacheControlparametersThis change aligns with the removal of metadata handling and route caching functionality, creating a more streamlined approach.
64-211: Verification needed for removed route cache functionalityThe overall function has been significantly simplified by removing the route caching and fetch cache management functionality. Based on the PR objective for Next.js 15 support, please ensure that these removals are intentional and that the functionality is either no longer needed or implemented elsewhere.
The documentation comments at the top of the function (lines 30-36) still mention that this populates routes and fetch calls, which are no longer supported. Please update the documentation to reflect the current functionality.
18-27:❓ Verification inconclusive
API simplification in RegisterInitialCacheOptions
The options type has been simplified by removing the previously available
fetchandroutesproperties, leaving only thepagesoption. This matches the documentation comments that were left intact and aligns with the apparent codebase simplification.Make sure that all consumers of this API have been updated to no longer pass the removed options.
🏁 Script executed:
#!/bin/bash # Search for any imports and usages of registerInitialCache that might still pass the removed options rg -A 3 -B 3 "registerInitialCache.*\{" --type ts --type jsLength of output: 61
Action Required: Perform Additional Manual Verification for Removed Options
The updated
RegisterInitialCacheOptionstype now only allows thepagesoption, with thefetchandroutesproperties intentionally removed. Our automatedrgsearch didn’t return any results for consumers still passing these removed options. However, since the command did not yield output and the inference quality is low, please manually verify that there are no residual usages of the removed options in the codebase—especially in less common scenarios or test files that might not match the initial pattern.
- Ensure that all invocations of
registerInitialCacheacross the repository no longer passfetchorroutes.- Double-check unit and integration tests for any references to these deprecated options.
- Confirm that the documentation and usage examples have been updated accordingly.
packages/cache-handler/src/cache-handler.ts (9)
14-17: Imports look good and remain consistent with the enumerations.
No issues flagged; this helps align with the typed constants across the codebase.
624-624: Switch tocomposeAgeEstimationFnis valid and consistent.
This refactor follows the new approach for estimating expiration ages.
829-833: Ensure correctness with mandatoryctxparameter and non-PAGES fallback.
This snippet now returnsnullifctx?.kindisn'tPAGES. Confirm that all.get()call sites pass the properctx(as previously recommended) and that ignoring non-PAGES entries is the intended behavior.
835-835: Configuration call appears logically placed.
Invoking#configureCacheHandler()only forPAGESis clear and consistent.
848-848: DefaultingimplicitTagsto an empty array is consistent with the metadata definition.
No concerns.
868-874: Confirm exclusion of non-PAGES orfetchCacheitems from caching.
This conditional short-circuits storing any data that isn’tPAGESor ifctx.fetchCacheis true. Verify that this is the correct and desired behavior.
887-887: Parameter destructuring looks correct.
Extractingneshca_lastModifiedandcacheControlmatches the new property usage.
903-908: Object construction for the cache entry is appropriately updated.
Storingtags: []andvalue: incrementalCacheValuealigns with the new enumerated approach forPAGES.
912-912: Conditionally writing pages to disk based onfallback: falseis consistent.
This ensures file system caching is only applied where needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
.vscode/settings.json (1)
8-8: Explicit TypeScript SDK Path Configuration Added
The new entry"typescript.tsdk": "node_modules/typescript/lib"ensures that VS Code uses your project's locally installed TypeScript version. Please verify that this relative path correctly points to the intended TypeScript SDK directory in your repository.packages/cache-handler/src/cache-handler.ts (3)
838-838: Configuring the cache before fetching is sensible.
Be mindful of any performance overhead if this is invoked repeatedly. You might consider caching the configuration after the first invocation.
863-868: Consider refactoring fallback logic to avoid duplication.
Currently, page data is read again ifctx.isFallbackis true. You can combine this logic with the check at line 855 for a cleaner flow.if (CacheHandler.#fallbackFalseRoutes.has(cacheKey) || ctx.isFallback) { cachedData = await CacheHandler.#readPagesRouterPage( cacheKey, - false, + !!ctx.isFallback ); }
930-931:revalidateTag()remains unimplemented.
We can help finalize a tag-based invalidation mechanism if needed. Let me know if you'd like to open an issue to track implementing this method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.vscode/settings.json(2 hunks)apps/cache-testing/tests/pages.spec.ts(0 hunks)packages/cache-handler/src/cache-handler.ts(10 hunks)packages/cache-handler/src/use-cache/redis.ts(1 hunks)turbo.json(2 hunks)
💤 Files with no reviewable changes (1)
- apps/cache-testing/tests/pages.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- turbo.json
- packages/cache-handler/src/use-cache/redis.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: redis-strings
- GitHub Check: redis-stack
🔇 Additional comments (15)
.vscode/settings.json (2)
43-45: New HTML Default Formatter Configuration Added
The addition of the[html]section with"editor.defaultFormatter": "esbenp.prettier-vscode"aligns HTML formatting with your updated project standards. Ensure that this formatter matches your team’s expectations and that any HTML files are consistently formatted.
1-7: General Configuration Cleanup and Consistency Check
The removal of previous configurations (such asexplorer.fileNesting.patternsandgrammarly.selectors) appears intentional based on the broader project updates toward modern JavaScript/TypeScript practices. Please double-check that these removals do not impact any existing workflows or extensions your team relies on.Also applies to: 9-42
packages/cache-handler/src/cache-handler.ts (13)
14-15: Adoption of enumerations fromnext-common-types.jsis a solid step for maintainability.
No issues found. This addition enhances type safety compared to plain string constants.
17-17: Good update referencingcomposeAgeEstimationFn.
This aligns well with the switch to ES modules and ensures a more modular approach to calculating expiration ages.
421-425: Verify the fallback logic for JSON file omission.
WhenisFallbackis truthy, the.jsonfile is skipped. Ensure this is fully aligned with the intended behavior for fallback pages.
443-443: SwitchingkindtoCachedRouteKind.PAGESis a welcome type improvement.
This transition from a string literal to an enum clarifies intent and reduces errors.
627-627: Refactoring to usecomposeAgeEstimationFnfor stale age logic looks clean.
This improves extensibility by allowing a pluggable age estimation function.
834-836: Confirm restricting thegetmethod toIncrementalCacheKind.PAGES.
The short-circuit might skip caching for other route kinds. Verify if this is intentional or if fallback logic for other route types is needed.
851-851: EmptyimplicitTagsarray could limit future tag-based invalidation.
Confirm whether you plan to add tags later or keep them empty by design.
855-855: Falling back to a file system read for non-cached pages matches Next.jsfallback: falsebehavior.
Looks correct for pages that must be served from disk when not found in the cache.
879-883: Validate the early return condition for non-PAGESroutes orfetchCacheusage.
Ensure it’s correct to skip storing such entries. Some workflows might still want partial caching.
896-897: Extractingneshca_lastModifiedandcacheControlfromctxlooks fine.
No issues found; this is straightforward destructuring.
898-898: Verify behavior whencacheControlis undefined.
revalidatewill be undefined in that scenario, so confirm that case is acceptable for your flow.
913-918: Emptytagsarray might lose track of potential tag associations.
Confirm if ignoring or discarding tags is intended. Otherwise, consider preserving relevant tags incacheHandlerValue.
922-922: Conditional file write for fallback false routes is aligned with Next.js semantics.
No further concerns here.
| @@ -829,11 +829,13 @@ export class CacheHandler implements NextCacheHandler { | |||
|
|
|||
| async get( | |||
| cacheKey: CacheHandlerParametersGet[0], | |||
| ctx: CacheHandlerParametersGet[1] = {}, | |||
| ctx: CacheHandlerParametersGet[1], | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
ctx as a required parameter is a breaking change.
Please confirm that all call sites now pass the ctx argument to avoid runtime errors.
🏁 Script executed:
#!/bin/bash
# Locate calls to the get method across the codebase, ensuring a second argument for ctx
rg -A 3 '\.get\(' --type tsLength of output: 6598
Action Required: Ensure ctx Parameter is Passed in All CacheHandler.get Calls
The recent change in the cache-handler.ts API makes the ctx argument mandatory when calling the get method. Our verification shows that while some call sites (e.g., in cache-handler.ts) correctly pass the required second argument (like await handler.get(key, meta) and await CacheHandler.#mergedHandler.get(cacheKey, { implicitTags: [] })), other call sites—most notably in packages/cache-handler/src/use-cache/redis.ts (e.g., await pendingSets.get(cacheKey);)—do not provide a ctx argument.
Please review and update the following:
-
packages/cache-handler/src/use-cache/redis.ts- Confirm whether
pendingSets.get(cacheKey)should now be called with an addedctxargument.
- Confirm whether
-
Other related modules (such as in
handlers/local-lru.tsif applicable)- Check if their
getcalls are intended to use the updated API and ensure they pass the required context.
- Check if their
Addressing these inconsistencies is crucial to avoid runtime errors due to the breaking change in the API.
better-salmon
force-pushed
the
upgrade-next-js
branch
from
5f76eb2 to
c49268e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/cache-handler/src/use-cache/redis.ts (2)
394-406: Reading the entire stream into memory may cause performance bottlenecks.
Accumulating all chunks before storing them in Redis can be expensive for large streams, potentially leading to high memory usage. If inputs can be large, consider chunking or an alternative storage approach.
1-529: Add or extend test coverage for this file.
This file implements complex logic (e.g., streaming, tag invalidation, pub/sub, concurrency withpendingSets). Thorough tests will help prevent regressions and ensure all edge cases, such as Redis downtime or large data streams, are handled.Would you like me to propose a set of Jest or integration tests to validate these features?
packages/cache-handler/src/cache-handler.ts (1)
930-931:revalidateTagis not yet implemented
A placeholder is present forrevalidateTag. If this feature is required, consider adding an actual implementation or removing the method until needed.Do you want me to open a follow-up issue or draft an implementation for tag-based revalidation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
.vscode/settings.json(2 hunks)apps/cache-testing/package.json(2 hunks)apps/cache-testing/run-app-instances.ts(1 hunks)apps/cache-testing/tests/pages.spec.ts(0 hunks)docs/cache-handler-docs/package.json(1 hunks)internal/eslint-config/package.json(1 hunks)packages/cache-handler/src/cache-handler.ts(10 hunks)packages/cache-handler/src/use-cache/redis.ts(1 hunks)turbo.json(2 hunks)
💤 Files with no reviewable changes (1)
- apps/cache-testing/tests/pages.spec.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/cache-testing/run-app-instances.ts
- turbo.json
- .vscode/settings.json
- internal/eslint-config/package.json
- docs/cache-handler-docs/package.json
- apps/cache-testing/package.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: redis-stack
- GitHub Check: redis-strings
🔇 Additional comments (12)
packages/cache-handler/src/use-cache/redis.ts (3)
1-8: Usage of internal Next.js modules is risky.
This import references internal Next.js modulescache-handlers/types.jsandincremental-cache/tags-manifest.external.js, which are undocumented and prone to breaking changes in Next.js 15+. Prefer documented caching interfaces (e.g.,cacheHandlerconfig,revalidateTag) to ensure forward compatibility.
331-344: Consider verifying the usage ofperformance.timeOrigin + performance.now().
Usingperformance.timeOrigin + performance.now()for expiration checks might introduce issues if the Node.js environment lacks consistent real-time alignment withDate.now(), especially under serverless or container restarts. Consider usingDate.now()consistently to simplify time comparisons.Would you like a script to search for any other instances where
performanceis used for time tracking?
300-303: Review concurrency approach.
WhilependingSetshelps serialize concurrent writes to the same cache key, make sure your usage model correctly handles high concurrency or frequent reads-and-writes to the same key. Tests should confirm that multiple concurrent requests waiting on a pending set operation do not cause data loss or race conditions.Also applies to: 364-377
packages/cache-handler/src/cache-handler.ts (9)
14-15: Good import adjustments for enumerated types.
Revising the import to bring inCachedRouteKindandIncrementalCacheKindaligns with their usage in the code below.
627-627: Replacing the old estimation function withcomposeAgeEstimationFn
UsingcomposeAgeEstimationFnensures a more flexible approach and is consistent with the new design.
832-838: Confirm that all call sites provide the new requiredctxparam.
Makingctxmandatory and returningnullunlessctx.kind === IncrementalCacheKind.PAGESis a breaking change. As previously noted, please verify that everyCacheHandler.getcall passes a compatible context.#!/bin/bash # Verify that all get calls have the second argument rg -A 2 '\.get\(' --type ts
851-851: InitializingimplicitTagsto an empty array
This is a clear approach to ensure no default tags are used. No immediate concerns.
855-855: Fetching from file system for fallback-false routes
Good approach to populate cache misses from the filesystem. This ensures consistency when the route is markedfallback: false.
896-898: ExtractingrevalidatefromcacheControl
Destructuringneshca_lastModifiedand referencingcacheControl?.revalidateis straightforward. Confirm thatneshca_lastModifiedis reliably set in all relevant contexts.
913-918: Constructing the cache value object
Replacing references to any priorsoftTagsor dynamic tags with a static empty array is fine if tags are not used. Ensure that any future expansions to caching logic handle tags properly.
863-868:✅ Verification successful
Handle fallback pages directly from the file system
Adding logic to read from disk ifctx.isFallbackis true covers fallback scenarios for the Pages Router. Verify that the code cannot conflict with the line 855 fallback logic.
🏁 Script executed:
#!/bin/bash # Checks if new logic with both fallback:false and fallback:true might conflict rg -A 5 'isFallback'Length of output: 3212
Fallback pages logic verified – no conflicts detected.
After reviewing the search results, the fallback handling via the conditional block using
ctx.isFallbackis distinct and does not interfere with the logic around line 855. Specifically:
- In the debug block (line 855), the ternary operator returns an empty object when
isFallbackis true.- In the new block (lines 863–868), when
ctx.isFallbackis true, the code retrieves fallback pages usingCacheHandler.#readPagesRouterPage.Both branches are mutually exclusive, ensuring that fallback pages are correctly handled without conflict.
879-883:✅ Verification successful
Skipping cache set for non-PAGES or fetchCache flows
Conditionally returning early ifincrementalCacheValue.kindis notPAGESor ifctx.fetchCacheis true may be intentional. Confirm you don’t need to store other route kinds.
🏁 Script executed:
#!/bin/bash # Inspect references to incrementalCacheValue.kind across code rg -A 4 'incrementalCacheValue\.kind'Length of output: 39
I'll run an additional search to inspect how
CachedRouteKindis referenced across the repository. This will help us verify if any non‑PAGES route kinds are being used elsewhere and whether the early return is truly appropriate.
🏁 Script executed:
#!/bin/bash # Searching for references to CachedRouteKind (including .PAGES) across the repository for better context rg -n --context 4 'CachedRouteKind\.' .Length of output: 5569
Clarifying cache-setting logic for CachedRouteKind
It appears that the early return in
cache-handler.ts(lines 879–883) intentionally bypasses caching for routes whose kind is notCachedRouteKind.PAGES(or whenctx.fetchCache === true). The repository scan confirms that this file only handles the PAGES kind—other kinds (e.g., REDIRECT, IMAGE, FETCH, APP_ROUTE, APP_PAGE) are processed elsewhere (for example, in the switch statement inhandlers/local-lru.ts). Please confirm that this split in responsibilities is intentional and that caching for non‑PAGES routes is indeed managed by other components.
|
|
||
| import { createValidatedAgeEstimationFunction } from './helpers/create-validated-age-estimation-function'; | ||
| import { getTagsFromHeaders } from './helpers/get-tags-from-headers'; | ||
| import { composeAgeEstimationFn } from './utils/compose-age-estimation-fn.js'; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure the utility import path is correct.
The new composeAgeEstimationFn utility import looks relevant. Confirm that ./utils/compose-age-estimation-fn.js is the intended path and that no references remain to any removed functions.
🏁 Script executed:
#!/bin/bash
# Verify that no references to deprecated or old age estimation functions remain
rg "createValidatedAgeEstimationFunction"Length of output: 41
I'm going to run an additional verification script to confirm that no deprecated function references remain and that the new utility file exists at the intended path. This script checks for any occurrences of "createValidatedAgeEstimationFunction" across the repository and verifies that the file compose-age-estimation-fn.js exists under packages/cache-handler/src/utils.
Please run this script and share the output so we can conclusively verify that:
- The file import path (
./utils/compose-age-estimation-fn.js) is correct. - No obsolete references remain in the codebase.
🏁 Script executed:
#!/bin/bash
echo "Searching for any reference to 'createValidatedAgeEstimationFunction':"
rg -n "createValidatedAgeEstimationFunction" || echo "No references found."
echo ""
echo "Checking for 'compose-age-estimation-fn.js' existence in packages/cache-handler/src/utils:"
find packages/cache-handler/src/utils -type f | grep "compose-age-estimation-fn.js" || echo "compose-age-estimation-fn.js not found in the expected path."Length of output: 675
Action Required: Update the Utility Import Path
The verification shows that no deprecated functions remain, but the file compose-age-estimation-fn.js was not found under packages/cache-handler/src/utils. Please verify that this file exists at the intended path. If the new utility is located elsewhere or has been renamed, update the import statement accordingly.
- Confirm if
compose-age-estimation-fn.jswas moved or renamed. - Update the import path in
packages/cache-handler/src/cache-handler.tsif necessary.
|
@better-salmon, I'd be willing to take on the remaining of this PR and contribute would just like to align on what the remaining work is before I dive deep |
|
let's make sure to show a little bit of appreciation for the great work Arseny has done so far 😊 https://opencollective.com/caching-tools#category-CONTRIBUTE @Pruxis @ricotijsen @Rikard-Johansson97 @iz-podpolja @ishida0210 @Edge1S-mgawlik @druckmax @MrBuBBLs @QurcoJr @Andr1yK |
|
@jobenjada |
|
Very nice work, if there are some more things where we can help with development/test, let's make it happen :) |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
apps/cache-testing/package.json(1 hunks)docs/cache-handler-docs/package.json(1 hunks)internal/backend/package.json(1 hunks)internal/eslint-config/package.json(1 hunks)package.json(1 hunks)packages/cache-handler/package.json(2 hunks)pnpm-workspace.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/backend/package.json
- pnpm-workspace.yaml
- docs/cache-handler-docs/package.json
- packages/cache-handler/package.json
- internal/eslint-config/package.json
- package.json
🔇 Additional comments (5)
apps/cache-testing/package.json (5)
6-6: Verify ESM module configuration
Adding"type": "module"will enforce ES module semantics for all.jsfiles. Please ensure that files likecluster.config.jsor other scripts intended to remain CommonJS are either renamed to.cjsor updated to valid ESM syntax.
8-8: Ensure build script dependencies are available
The build script now usestsx create-instances.tsinstead of the shell script. Confirm thatcreate-instances.tsexists at the project root and thattsxis installed in CI environments before runningbuild.
13-14: Update documentation and CI for lint scripts
The scripts have been renamed fromeslint:check/eslint:fixtolint:check/lint:fixwith--max-warnings=0. Remember to update any documentation, project README, or CI workflows that reference the old script names.
20-23: Upgrade runtime dependencies for Next.js 15 support
Bumpingaxios,next,react, andreact-domaligns with the PR goal of Next.js 15 compatibility. Good to see versions updated. Ensure that any peer dependencies or lock files are updated accordingly.
27-27: Apply consistent dev dependency upgrades
Dev dependencies such as Playwright, TypeScript, ESLint, and their type definitions have been updated to the latest compatible versions, keeping the development environment standardized.Also applies to: 30-32, 34-38
| "@types/node": "22.15.18", | ||
| "@types/react": "19.1.4", | ||
| "@types/react-dom": "19.1.5", | ||
| "dotenv-cli": "8.0.0", |
There was a problem hiding this comment.
Critical: Move dotenv-cli to production dependencies or adjust start script
The start script relies on the dotenv command from dotenv-cli, which is currently in devDependencies. In production (NODE_ENV=production), dev dependencies may not be installed, causing start to fail. Consider moving dotenv-cli to dependencies or handling environment variables at runtime via CI or a lightweight wrapper.
🤖 Prompt for AI Agents
In apps/cache-testing/package.json at line 33, the `dotenv-cli` package is
listed under devDependencies but is required by the start script in production.
To fix this, move `dotenv-cli` from devDependencies to dependencies to ensure it
is installed in production environments, or alternatively, modify the start
script to handle environment variables without relying on `dotenv-cli` at
runtime.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
packages/cache-handler/src/handlers/redis-stack.ts (1)
230-232:⚠️ Potential issueKey prefix is missing in
delete– item will never be removed
This is the same issue flagged in earlier reviews for theredis-stringshandler.-await client.withAbortSignal(AbortSignal.timeout(timeoutMs)).unlink(key); +await client + .withAbortSignal(AbortSignal.timeout(timeoutMs)) + .unlink(keyPrefix + key);packages/cache-handler/src/handlers/experimental-redis-cluster.ts (1)
270-274:⚠️ Potential issueKey prefix omitted in cluster
deleteThe data was stored with
keyPrefix + key; deleting without the prefix will orphan the entry.-.unlink(key); +.unlink(keyPrefix + key);
🧹 Nitpick comments (3)
packages/cache-handler/package.json (2)
33-40: Refine build and dev scripts & remove unused devDependency
- The
tsxpackage is no longer referenced in any script; you can drop it to slim down installs.- Consider adding a
prepareorprepublishOnlyhook to run"build"and"check-types"automatically before publishing.Example adjustments:
"scripts": { - "build": "tsc -p tsconfig.build.json", + "build": "tsc -p tsconfig.build.json", "check-types": "tsc --noEmit", - "dev": "tsc -w -p tsconfig.build.json", + "dev": "tsc -w -p tsconfig.build.json", "lint:check": "eslint . --max-warnings=0", "lint:fix": "eslint --fix . --max-warnings=0", "test": "vitest run", "test:watch": "vitest", + "prepare": "npm run build && npm run check-types" },And remove
tsxfromdevDependenciesif unused.
57-59: Review peer dependency ranges
Lockingnextto>=15 <16is appropriate for Next.js 15 support. Forredis, confirm that you don’t need to support v6+ (e.g., for compatibility with newer Redis clients). Adjust the range if broader compatibility is required.packages/cache-handler/src/handlers/redis-stack.ts (1)
160-181: Optional: filter out undefined promises inPromise.allWhen
expireCacheValueisundefined, it is still passed toPromise.all; this resolves fine but is a small smell. Consider filtering falsy values for clarity.-await Promise.all([setCacheValue, expireCacheValue]); +await Promise.all([setCacheValue, expireCacheValue].filter(Boolean));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
apps/cache-testing/package.json(1 hunks)internal/eslint-config/vitest.js(1 hunks)packages/cache-handler/package.json(2 hunks)packages/cache-handler/src/handlers/experimental-redis-cluster.ts(8 hunks)packages/cache-handler/src/handlers/redis-stack.ts(7 hunks)packages/cache-handler/src/handlers/redis-strings.ts(8 hunks)packages/cache-handler/src/helpers/helpers.ts(1 hunks)packages/cache-handler/src/use-cache/node-redis.ts(1 hunks)packages/cache-handler/test/utils/compose-age-estimation-fn.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/eslint-config/vitest.js
- packages/cache-handler/test/utils/compose-age-estimation-fn.test.ts
- packages/cache-handler/src/use-cache/node-redis.ts
- packages/cache-handler/src/handlers/redis-strings.ts
- apps/cache-testing/package.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: redis-stack
- GitHub Check: redis-strings
🔇 Additional comments (3)
packages/cache-handler/package.json (2)
42-45: Dependencies updates look good
The bump to caret ranges forcluster-key-slotandlru-cache, plus the introduction ofsuperjson, align with the modernization effort.
47-55: Verify workspace protocol compatibility
You’ve switched toworkspace:*for local ESLint and TS configs. Ensure that your chosen package manager (npm, Yarn, pnpm) supports this notation in published packages, or consider replacing it with explicit relative paths or versioned entries.packages/cache-handler/src/helpers/helpers.ts (1)
1-1:✅ Verification successful
Export barrel now exposes only one helper – double-check downstream imports
All other helpers (
promiseWithTimeout,getTimeoutRedisCommandOptions, etc.) were removed from this barrel. If any consumer code (internal or external) still imports them frompackages/cache-handler/src/helpers, it will now fail at runtime. Please run a repo-wide grep before merging to ensure no lingering imports remain.Example verification command:
🏁 Script executed:
rg "from ['\"].*helpers['\"]" --glob '!dist/**'Length of output: 251
No lingering imports of removed helpers found
I ran a repo-wide grep for imports from “…/helpers” and only found:
apps/cache-testing/tests/app.spec.tsimporting from./test-helpers(unrelated)docs/cache-handler-docs/src/app/usage/creating-a-custom-handler/page.mdximportingisTagImplicit(the only remaining export)Since no other helpers are being imported from
packages/cache-handler/src/helpers, there are no runtime breakages.
| @@ -14,7 +14,7 @@ | |||
| "bugs": "https://github.com/caching-tools/next-shared-cache/issues", | |||
| "repository": { | |||
| "type": "git", | |||
| "url": "git+https://github.com/caching-tools/next-shared-cache.git" | |||
| "url": "git+https://github.com/caching-tools/next-shared-cache.git " | |||
There was a problem hiding this comment.
Fix trailing whitespace in the Git repository URL
The value for "url" has an extra space at the end, which can cause failures when installing or cloning the package.
Apply this diff to remove the trailing space:
- "url": "git+https://github.com/caching-tools/next-shared-cache.git "
+ "url": "git+https://github.com/caching-tools/next-shared-cache.git"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "url": "git+https://github.com/caching-tools/next-shared-cache.git " | |
| "url": "git+https://github.com/caching-tools/next-shared-cache.git" |
🤖 Prompt for AI Agents
In packages/cache-handler/package.json at line 17, remove the trailing
whitespace at the end of the Git repository URL value for the "url" field to
ensure the URL is correctly formatted and avoid installation or cloning errors.
| ".": "./dist/cache-handler.js", | ||
| "./helpers/*": "./dist/helpers/*.js", | ||
| "./handlers/*": "./dist/handlers/*.js", | ||
| "./instrumentation/*": "./dist/instrumentation/*.js", | ||
| "./use-cache/*": "./dist/use-cache/*.js" | ||
| }, |
There was a problem hiding this comment.
Add TypeScript declaration exports
You’ve consolidated the ESM exports nicely, but consumers of this package won’t be able to locate the generated .d.ts files. It’s critical to expose type definitions so that TypeScript projects can statically validate imports.
Consider extending your exports with a dual-field format or adding a top-level "types" entry. For example:
"exports": {
- ".": "./dist/cache-handler.js",
+ ".": {
+ "import": "./dist/cache-handler.js",
+ "types": "./dist/cache-handler.d.ts"
+ },
"./helpers/*": "./dist/helpers/*.js",
"./handlers/*": "./dist/handlers/*.js",
"./instrumentation/*": "./dist/instrumentation/*.js",
"./use-cache/*": "./dist/use-cache/*.js"
},
+ "types": "dist/cache-handler.d.ts",This ensures editors and compilers pick up the declarations correctly.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ".": "./dist/cache-handler.js", | |
| "./helpers/*": "./dist/helpers/*.js", | |
| "./handlers/*": "./dist/handlers/*.js", | |
| "./instrumentation/*": "./dist/instrumentation/*.js", | |
| "./use-cache/*": "./dist/use-cache/*.js" | |
| }, | |
| ".": { | |
| "import": "./dist/cache-handler.js", | |
| "types": "./dist/cache-handler.d.ts" | |
| }, | |
| "./helpers/*": "./dist/helpers/*.js", | |
| "./handlers/*": "./dist/handlers/*.js", | |
| "./instrumentation/*": "./dist/instrumentation/*.js", | |
| "./use-cache/*": "./dist/use-cache/*.js" | |
| }, | |
| "types": "dist/cache-handler.d.ts", |
🤖 Prompt for AI Agents
In packages/cache-handler/package.json around lines 26 to 31, the exports field
lacks references to the TypeScript declaration (.d.ts) files, which prevents
TypeScript consumers from accessing type definitions. To fix this, extend the
exports entries to include a "types" or "typings" field pointing to the
corresponding .d.ts files alongside the JavaScript files, or add a top-level
"types" entry referencing the main declaration file. This will enable editors
and TypeScript compilers to locate and use the type declarations properly.
| const signal = AbortSignal.timeout(timeoutMs); | ||
|
|
||
| await client.unlink(options, keysToDelete); | ||
| await client.withAbortSignal(signal).unlink(keysToDelete); | ||
| }, |
There was a problem hiding this comment.
unlink should be called with spread keys, not an array
unlink(keysToDelete) transmits a single argument (the array).
Redis will treat that as one key and delete nothing.
-await client.withAbortSignal(signal).unlink(keysToDelete);
+await client.withAbortSignal(signal).unlink(...keysToDelete);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const signal = AbortSignal.timeout(timeoutMs); | |
| await client.unlink(options, keysToDelete); | |
| await client.withAbortSignal(signal).unlink(keysToDelete); | |
| }, | |
| const signal = AbortSignal.timeout(timeoutMs); | |
| await client.withAbortSignal(signal).unlink(...keysToDelete); | |
| }, |
🤖 Prompt for AI Agents
In packages/cache-handler/src/handlers/redis-stack.ts around lines 226 to 229,
the unlink method is called with an array argument keysToDelete, which causes
Redis to treat it as a single key. To fix this, spread the keysToDelete array
when calling unlink so that each key is passed as a separate argument, ensuring
all keys are properly deleted.
| const revalidationTimes = await client | ||
| .withAbortSignal(AbortSignal.timeout(timeoutMs)) | ||
| .hmGet(revalidatedTagsKey, Array.from(combinedTags)); | ||
|
|
There was a problem hiding this comment.
hmGet receives an array – API expects variadic fields
redis v4’s signature is hmGet(key, ...fields).
Passing an array results in a single stringified field lookup, so all revalidation checks silently fail.
-const revalidationTimes = await client
- .withAbortSignal(AbortSignal.timeout(timeoutMs))
- .hmGet(revalidatedTagsKey, Array.from(combinedTags));
+const revalidationTimes = await client
+ .withAbortSignal(AbortSignal.timeout(timeoutMs))
+ .hmGet(revalidatedTagsKey, ...combinedTags);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const revalidationTimes = await client | |
| .withAbortSignal(AbortSignal.timeout(timeoutMs)) | |
| .hmGet(revalidatedTagsKey, Array.from(combinedTags)); | |
| const revalidationTimes = await client | |
| .withAbortSignal(AbortSignal.timeout(timeoutMs)) | |
| .hmGet(revalidatedTagsKey, ...combinedTags); |
🤖 Prompt for AI Agents
In packages/cache-handler/src/handlers/redis-stack.ts around lines 140 to 143,
the call to hmGet incorrectly passes an array of fields instead of variadic
arguments. To fix this, spread the array of combinedTags using the spread
operator (...) so that each tag is passed as a separate argument to hmGet,
matching the expected variadic parameters of the redis v4 API.
| for (const [slot, keys] of slotKeysMap) { | ||
| const targetMasterNode = cluster.slots[slot]?.master; | ||
| const client = await targetMasterNode?.client; | ||
|
|
||
| const client = targetMasterNode?.client; | ||
|
|
||
| if (keys.length === 0 || !client) { | ||
| continue; | ||
| } | ||
|
|
||
| const unlinkPromisesForSlot = client.unlink( | ||
| getTimeoutRedisCommandOptions(timeoutMs), | ||
| keys, | ||
| unlinkPromises.push( | ||
| client.withAbortSignal(AbortSignal.timeout(timeoutMs)).unlink(keys), | ||
| ); | ||
|
|
||
| if (unlinkPromisesForSlot) { | ||
| unlinkPromises.push(unlinkPromisesForSlot); | ||
| } | ||
| } |
There was a problem hiding this comment.
Cluster unlink uses array instead of variadic keys
Each individual master node client needs unlink(...keys) not unlink(keys):
-unlinkPromises.push(
- client.withAbortSignal(AbortSignal.timeout(timeoutMs)).unlink(keys),
-);
+unlinkPromises.push(
+ client.withAbortSignal(AbortSignal.timeout(timeoutMs)).unlink(...keys),
+);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const [slot, keys] of slotKeysMap) { | |
| const targetMasterNode = cluster.slots[slot]?.master; | |
| const client = await targetMasterNode?.client; | |
| const client = targetMasterNode?.client; | |
| if (keys.length === 0 || !client) { | |
| continue; | |
| } | |
| const unlinkPromisesForSlot = client.unlink( | |
| getTimeoutRedisCommandOptions(timeoutMs), | |
| keys, | |
| unlinkPromises.push( | |
| client.withAbortSignal(AbortSignal.timeout(timeoutMs)).unlink(keys), | |
| ); | |
| if (unlinkPromisesForSlot) { | |
| unlinkPromises.push(unlinkPromisesForSlot); | |
| } | |
| } | |
| for (const [slot, keys] of slotKeysMap) { | |
| const targetMasterNode = cluster.slots[slot]?.master; | |
| const client = targetMasterNode?.client; | |
| if (keys.length === 0 || !client) { | |
| continue; | |
| } | |
| unlinkPromises.push( | |
| client.withAbortSignal(AbortSignal.timeout(timeoutMs)).unlink(...keys), | |
| ); | |
| } |
🤖 Prompt for AI Agents
In packages/cache-handler/src/handlers/experimental-redis-cluster.ts around
lines 250 to 262, the unlink method is called with a single array argument
(unlink(keys)) but it requires variadic arguments (unlink(...keys)). To fix
this, spread the keys array when calling unlink by changing unlink(keys) to
unlink(...keys) so each key is passed as a separate argument.
| .withCommandOptions({ abortSignal: AbortSignal.timeout(timeoutMs) }) | ||
| .hDel(keyPrefix + sharedTagsKey, tagsToDelete); | ||
|
|
There was a problem hiding this comment.
hDel must receive spread fields
-.hDel(keyPrefix + sharedTagsKey, tagsToDelete);
+.hDel(keyPrefix + sharedTagsKey, ...tagsToDelete);🤖 Prompt for AI Agents
In packages/cache-handler/src/handlers/experimental-redis-cluster.ts around
lines 265 to 267, the hDel command is called with tagsToDelete as a single
argument, but it requires the fields to be passed as separate arguments. Fix
this by spreading the tagsToDelete array or iterable into individual arguments
when calling hDel, ensuring each field to delete is passed separately.
| const revalidationTimes = await cluster | ||
| .withCommandOptions({ abortSignal: AbortSignal.timeout(timeoutMs) }) | ||
| .hmGet(revalidatedTagsKey, Array.from(combinedTags)); |
There was a problem hiding this comment.
hmGet array issue – same as in redis-stack
Spread the tag array so each field is sent separately:
-.hmGet(revalidatedTagsKey, Array.from(combinedTags));
+.hmGet(revalidatedTagsKey, ...combinedTags);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const revalidationTimes = await cluster | |
| .withCommandOptions({ abortSignal: AbortSignal.timeout(timeoutMs) }) | |
| .hmGet(revalidatedTagsKey, Array.from(combinedTags)); | |
| const revalidationTimes = await cluster | |
| .withCommandOptions({ abortSignal: AbortSignal.timeout(timeoutMs) }) | |
| .hmGet(revalidatedTagsKey, ...combinedTags); |
🤖 Prompt for AI Agents
In packages/cache-handler/src/handlers/experimental-redis-cluster.ts around
lines 125 to 127, the hmGet command is called with an array of tags as a single
argument, but it requires each tag to be passed as a separate argument. Fix this
by spreading the combinedTags array so that each tag is sent individually to
hmGet.
|
Would love to see this cross the line soon! We're looking to use redis clusters in a self hosted Next.js setup, and this would make the approach much easier than writing a custom solution. 🚀 |
We are welcoming anybody to try out: https://github.com/trieb-work/nextjs-turbo-redis-cache We saw around 80% less redis traffic with our approach and because in the long run also 90% less memory usage in Redis in comparison to our setup with neshca. |
@JannikZed would love to but unfortunately cannot due to
We are moving to app router but currently are in a hybrid state split across app and pages. We can't easily move off pages router without a shared cache, hence waiting for this to land. Is this something you plan to support? |
|
For people needing a fast switch with minimal breaking changes -> https://github.com/fortedigital/nextjs-cache-handler |
better-salmon
force-pushed
the
upgrade-next-js
branch
from
76abafa to
9fdb6a4
Compare
|
@better-salmon Is there anything that can be done from the community here to get this moving? I've been telling engineers at my work that this is coming soon - almost done - for several months now. I don't see a migration path to a community replacement (https://github.com/fortedigital/nextjs-cache-handler, https://github.com/trieb-work/nextjs-turbo-redis-cache) that supports pages router, so feel rather stuck. If there are any outstanding tasks, that could help get this released, it would be great to align the community so those who want to could try and contribute 🙏🏻 |
|
@harry-gocity fyi even though most users who decided to go with https://github.com/fortedigital/nextjs-cache-handler use app router, the library has recently had compatibility fixes for Next 15 with pages router as well. |
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Tests
Style
Documentation