Skip to content

added fsConfig for cacheHandler #889

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: canary
Choose a base branch
from
Open

added fsConfig for cacheHandler #889

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .idea/.gitignore
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions .idea/inspectionProfiles/Project_Default.xml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions .idea/modules.xml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 12 additions & 0 deletions .idea/next-shared-cache.iml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions .idea/prettier.xml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions .idea/vcs.xml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

36 changes: 28 additions & 8 deletions packages/cache-handler/src/cache-handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { promises as fsPromises } from 'node:fs';
import {Mode, PathLike, promises as fsPromises} from 'node:fs';
import path from 'node:path';

import type {
Expand Down Expand Up @@ -177,6 +177,20 @@ export type TTLParameters = {
estimateExpireAge(staleAge: number): number;
};

/**
* A Configuration options for file system management. Used for testing. By default, it's a regular fsPromise.
*
* @default fsPromise
*
* @since 1.9.0
*/
export type fsConfig = {
open: typeof fsPromises.open;
readFile: typeof fsPromises.readFile;
writeFile: typeof fsPromises.writeFile;
mkdir: typeof fsPromises.mkdir
}

Comment on lines +180 to +193
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refine the fsConfig type definition and documentation

The introduction of the fsConfig type to encapsulate file system operations is a good approach for enhancing flexibility and testability. However, consider the following improvements:

  • Include All Used Methods: Ensure that all file system methods used in CacheHandler are included in the fsConfig type. Methods like stat and close are used but are not defined in fsConfig.
  • Naming Convention: TypeScript types are typically named using PascalCase. Rename fsConfig to FsConfig for consistency.

Apply this diff to refine the type definition:

-export type fsConfig = {
+export type FsConfig = {
     open: typeof fsPromises.open;
     readFile: typeof fsPromises.readFile;
     writeFile: typeof fsPromises.writeFile;
     mkdir: typeof fsPromises.mkdir;
+    stat: typeof fsPromises.stat;
+    close: (handle: fsPromises.FileHandle) => Promise<void>;
 };
📝 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.

Suggested change
/**
* A Configuration options for file system management. Used for testing. By default, it's a regular fsPromise.
*
* @default fsPromise
*
* @since 1.9.0
*/
export type fsConfig = {
open: typeof fsPromises.open;
readFile: typeof fsPromises.readFile;
writeFile: typeof fsPromises.writeFile;
mkdir: typeof fsPromises.mkdir
}
/**
* A Configuration options for file system management. Used for testing. By default, it's a regular fsPromise.
*
* @default fsPromise
*
* @since 1.9.0
*/
export type FsConfig = {
open: typeof fsPromises.open;
readFile: typeof fsPromises.readFile;
writeFile: typeof fsPromises.writeFile;
mkdir: typeof fsPromises.mkdir;
stat: typeof fsPromises.stat;
close: (handle: fsPromises.FileHandle) => Promise<void>;
}

/**
* Configuration options for the {@link CacheHandler}.
*
Expand Down Expand Up @@ -374,6 +388,12 @@ export class CacheHandler implements NextCacheHandler {

static #serverDistDir: string;

static #fsConf: fsConfig = fsPromises;

static setFsConf(fsConf: fsConfig) {
CacheHandler.#fsConf = fsConf;
}

Comment on lines +391 to +396
Copy link

Choose a reason for hiding this comment

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

🧹 Nitpick (assertive)

Assess thread safety with mutable static properties

Modifying the static property #fsConf at runtime using setFsConf can lead to concurrency issues in a multi-threaded environment.

  • Potential Risk: If CacheHandler is used in concurrent contexts, changes to #fsConf could affect other instances unexpectedly.
  • Recommendation: Consider passing fsConf as a parameter to instance methods or using dependency injection to avoid shared mutable state.

🧹 Nitpick (assertive)

Ensure consistency in static property and method definitions

  • Update Types and Names: With the renaming of fsConfig to FsConfig, update the property and method accordingly.
  • Type Safety: Ensure that the default value assigned to #fsConf matches the FsConfig type.

Apply this diff to update the names:

-static #fsConf: fsConfig = fsPromises;
+static #fsConf: FsConfig = {
+  open: fsPromises.open,
+  readFile: fsPromises.readFile,
+  writeFile: fsPromises.writeFile,
+  mkdir: fsPromises.mkdir,
+  stat: fsPromises.stat,
+  close: (handle) => handle.close(),
+};

-static setFsConf(fsConf: fsConfig) {
+static setFsConf(fsConf: FsConfig) {
     CacheHandler.#fsConf = fsConf;
 }
📝 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.

Suggested change
static #fsConf: fsConfig = fsPromises;
static setFsConf(fsConf: fsConfig) {
CacheHandler.#fsConf = fsConf;
}
static #fsConf: FsConfig = {
open: fsPromises.open,
readFile: fsPromises.readFile,
writeFile: fsPromises.writeFile,
mkdir: fsPromises.mkdir,
stat: fsPromises.stat,
close: (handle) => handle.close(),
};
static setFsConf(fsConf: FsConfig) {
CacheHandler.#fsConf = fsConf;
}

static async #readPagesRouterPage(cacheKey: string): Promise<CacheHandlerValue | null> {
let cacheHandlerValue: CacheHandlerValue | null = null;
let pageHtmlHandle: fsPromises.FileHandle | null = null;
Expand All @@ -392,12 +412,12 @@ export class CacheHandler implements NextCacheHandler {
const pageHtmlPath = path.join(CacheHandler.#serverDistDir, 'pages', `${cacheKey}.html`);
const pageDataPath = path.join(CacheHandler.#serverDistDir, 'pages', `${cacheKey}.json`);

pageHtmlHandle = await fsPromises.open(pageHtmlPath, 'r');
pageHtmlHandle = await CacheHandler.#fsConf.open(pageHtmlPath, 'r');

const [pageHtmlFile, { mtimeMs }, pageData] = await Promise.all([
pageHtmlHandle.readFile('utf-8'),
pageHtmlHandle.stat(),
fsPromises.readFile(pageDataPath, 'utf-8').then((data) => JSON.parse(data) as object),
CacheHandler.#fsConf.readFile(pageDataPath, 'utf-8').then((data) => JSON.parse(data) as object),
Comment on lines +415 to +420
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Direct fsPromises usage found in register-initial-cache.ts

The verification revealed that while cache-handler.ts correctly uses #fsConf for file operations, there are multiple direct usages of fsPromises in packages/cache-handler/src/instrumentation/register-initial-cache.ts. These should be updated to use the configured file system interface for consistency:

  • register-initial-cache.ts:
    • Line 96: fsPromises.readFile(prerenderManifestPath, 'utf-8')
    • Line 145: fsPromises.stat(${pathToRouteFiles}.body)
    • Line 165: fsPromises.readFile(${pathToRouteFiles}.body)
    • Line 220: fsPromises.stat(${pathToRouteFiles}.html)
    • Line 307: fsPromises.readdir(fetchCacheDir)
    • Line 327: fsPromises.stat(filePath)
    • Line 343: fsPromises.readFile(...)
🔗 Analysis chain

Verify all fsPromises references are replaced with #fsConf

Ensure that all file system operations within CacheHandler use CacheHandler.#fsConf instead of fsPromises. Any remaining direct references to fsPromises could lead to inconsistent behavior and reduce the benefits of configuring fsConf.

Run the following script to check for any missed references:

Also applies to: 470-470, 473-474, 564-564, 597-597

Let me verify if there are any actual usages of fsPromises in the code beyond the type definitions and imports by checking the actual function calls.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all occurrences of fsPromises in cache-handler.ts

# Test: Search for 'fsPromises' in the file. Expect: No occurrences found.
rg 'fsPromises' packages/cache-handler/src/cache-handler.ts

Length of output: 386

Script:

#!/bin/bash
# Search for actual usage of fsPromises in function calls or operations
ast-grep --pattern 'fsPromises.$_($_)'

# Also check for any direct fsPromises usage
ast-grep --pattern 'fsPromises'

Length of output: 3001

]);

if (CacheHandler.#debug) {
Expand Down Expand Up @@ -447,11 +467,11 @@ export class CacheHandler implements NextCacheHandler {
const pageHtmlPath = path.join(CacheHandler.#serverDistDir, 'pages', `${cacheKey}.html`);
const pageDataPath = path.join(CacheHandler.#serverDistDir, 'pages', `${cacheKey}.json`);

await fsPromises.mkdir(path.dirname(pageHtmlPath), { recursive: true });
await CacheHandler.#fsConf.mkdir(path.dirname(pageHtmlPath), { recursive: true });

await Promise.all([
fsPromises.writeFile(pageHtmlPath, pageData.html),
fsPromises.writeFile(pageDataPath, JSON.stringify(pageData.pageData)),
CacheHandler.#fsConf.writeFile(pageHtmlPath, pageData.html),
CacheHandler.#fsConf.writeFile(pageDataPath, JSON.stringify(pageData.pageData)),
Comment on lines +473 to +474
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle potential JSON stringification errors

When writing pageData to a file, JSON.stringify could throw an error if the data contains circular references. Ensure that the data is serializable or handle errors appropriately.

Consider wrapping JSON.stringify in a try-catch block or sanitizing pageData before writing it.

]);

if (CacheHandler.#debug) {
Expand Down Expand Up @@ -541,7 +561,7 @@ export class CacheHandler implements NextCacheHandler {
let buildId: string | undefined;

try {
buildId = await fsPromises.readFile(path.join(serverDistDir, '..', 'BUILD_ID'), 'utf-8');
buildId = await CacheHandler.#fsConf.readFile(path.join(serverDistDir, '..', 'BUILD_ID'), 'utf-8');
} catch (_error) {
buildId = undefined;
}
Expand Down Expand Up @@ -574,7 +594,7 @@ export class CacheHandler implements NextCacheHandler {
}

try {
const prerenderManifestData = await fsPromises.readFile(
const prerenderManifestData = await CacheHandler.#fsConf.readFile(
path.join(serverDistDir, '..', 'prerender-manifest.json'),
'utf-8',
);
Expand Down