src/utils/fs.ts (2)

6-22: ⚡ Quick win

Verify the dynamic import pattern efficiency.

The fsPromiseModule promise is created once and awaited on every method call. While this works correctly (promises can be awaited multiple times), it adds overhead. Consider storing the resolved module instead.

♻️ Suggested optimization

 function createNodeFs(): UnpluginContextFs {
-  const fsPromiseModule = import('node:fs/promises')
+  let fsPromise: Promise<typeof import('node:fs/promises')> | undefined
   return {
     readFile: async (path, options) => {
-      const fs = await fsPromiseModule
+      fsPromise ??= import('node:fs/promises')
+      const fs = await fsPromise
       return fs.readFile(path, options)
     },
     stat: async (path, options) => {
-      const fs = await fsPromiseModule
+      fsPromise ??= import('node:fs/promises')
+      const fs = await fsPromise
       return fs.stat(path, options)
     },
     lstat: async (path, options) => {
-      const fs = await fsPromiseModule
+      fsPromise ??= import('node:fs/promises')
+      const fs = await fsPromise
       return fs.lstat(path, options)
     },
   }
 }

Or better, cache the resolved module:

 function createNodeFs(): UnpluginContextFs {
-  const fsPromiseModule = import('node:fs/promises')
+  let fsModule: typeof import('node:fs/promises') | undefined
+  async function getFs() {
+    if (!fsModule) {
+      fsModule = await import('node:fs/promises')
+    }
+    return fsModule
+  }
   return {
     readFile: async (path, options) => {
-      const fs = await fsPromiseModule
+      const fs = await getFs()
       return fs.readFile(path, options)
     },
     stat: async (path, options) => {
-      const fs = await fsPromiseModule
+      const fs = await getFs()
       return fs.stat(path, options)
     },
     lstat: async (path, options) => {
-      const fs = await fsPromiseModule
+      const fs = await getFs()
       return fs.lstat(path, options)
     },
   }
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/utils/fs.ts` around lines 6 - 22, The dynamic import promise
fsPromiseModule in createNodeFs is awaited on every call to readFile/stat/lstat
which adds unnecessary overhead; change the implementation to cache the resolved
module (e.g., keep a local variable like cachedFs or resolvedFs) and on the
first call await fsPromiseModule and store the module, then use the cached
module for subsequent calls in readFile, stat, and lstat to avoid repeated
awaits.

---

28-30: ⚡ Quick win

Type assertions suppress important type checking.

The type assertions (as UnpluginContextFs['readFile']) bypass TypeScript's type checking and could hide incompatibilities between pify's output and the expected UnpluginContextFs interface.

Consider adding runtime validation or using more specific typing from pify.

💡 Alternative approach

Instead of type assertions, explicitly type the pified object:

 export function createBuildContextFs(inputFs?: InputFileSystem | null): UnpluginContextFs {
-  const fs = inputFs ? pify(inputFs) : createNodeFs()
+  const fs = inputFs ? pify(inputFs) as {
+    readFile: (path: PathLike, options?: any) => Promise<string | Uint8Array>
+    stat: (path: PathLike, options?: any) => Promise<Stats>
+    lstat: (path: PathLike, options?: any) => Promise<Stats>
+  } : createNodeFs()
 
   return {
-    readFile: fs.readFile as UnpluginContextFs['readFile'],
-    stat: fs.stat as UnpluginContextFs['stat'],
-    lstat: fs.lstat as UnpluginContextFs['lstat'],
+    readFile: fs.readFile,
+    stat: fs.stat,
+    lstat: fs.lstat,
   }
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/utils/fs.ts` around lines 28 - 30, The current lines use type assertions
(readFile: fs.readFile as UnpluginContextFs['readFile'], stat, lstat) which
silence mismatches between pify(fs) output and the UnpluginContextFs shape;
replace the assertions by explicitly typing the pify call (e.g.
pify<UnpluginContextFs>(fs)) or by creating a typed wrapper that maps and
validates each function (readFile, stat, lstat) to the expected signatures, and
add minimal runtime checks (typeof fn === 'function' and argument/return shape
checks or try/catch) to surface incompatible implementations rather than
suppressing errors with "as". Ensure you update the assignment site where
pify(fs) is produced so the resulting object already conforms to
UnpluginContextFs instead of asserting per-property.

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/utils/fs.ts`:
- Around line 6-22: The dynamic import promise fsPromiseModule in createNodeFs
is awaited on every call to readFile/stat/lstat which adds unnecessary overhead;
change the implementation to cache the resolved module (e.g., keep a local
variable like cachedFs or resolvedFs) and on the first call await
fsPromiseModule and store the module, then use the cached module for subsequent
calls in readFile, stat, and lstat to avoid repeated awaits.
- Around line 28-30: The current lines use type assertions (readFile:
fs.readFile as UnpluginContextFs['readFile'], stat, lstat) which silence
mismatches between pify(fs) output and the UnpluginContextFs shape; replace the
assertions by explicitly typing the pify call (e.g. pify<UnpluginContextFs>(fs))
or by creating a typed wrapper that maps and validates each function (readFile,
stat, lstat) to the expected signatures, and add minimal runtime checks (typeof
fn === 'function' and argument/return shape checks or try/catch) to surface
incompatible implementations rather than suppressing errors with "as". Ensure
you update the assignment site where pify(fs) is produced so the resulting
object already conforms to UnpluginContextFs instead of asserting per-property.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6b657dd7-39c0-455e-939a-11fe181bfc0c

Reviewing files that changed from the base of the PR and between 1623b46 and 953ab32.

• pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml

• docs/guide/index.md
• package.json
• pnpm-workspace.yaml
• src/bun/utils.ts
• src/esbuild/utils.ts
• src/farm/context.ts
• src/rspack/context.ts
• src/types.ts
• src/utils/fs.ts
• src/webpack/context.ts
• test/unit-tests/bun/utils.test.ts
• test/unit-tests/esbuild/utils.test.ts
• test/unit-tests/farm/context.test.ts
• test/unit-tests/resolve-id/resolve-id.test.ts
• test/unit-tests/rspack/context.test.ts
• test/unit-tests/webpack/context.test.ts

Pull request overview

Adds a standardized this.fs interface to Unplugin’s unified build context so plugins can perform basic file operations across bundlers (and use bundler-provided virtual/input FS where available), addressing the portability gap described in #593.

Changes:

• Introduces UnpluginBuildContext.fs (readFile, stat, lstat) in src/types.ts.
• Implements createBuildContextFs with Webpack/Rspack input FS support (promisified) and a Node fallback, and wires it into webpack/rspack/farm/esbuild/bun contexts.
• Updates unit tests and docs to assert/describe this.fs support across bundlers.

Reviewed changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 4 comments.

• pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.