Skip to content

fix(fs): exclude ignored file in key listing #622

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
odzhychko wants to merge 1 commit into
base: main
Choose a base branch
from
+36 −3
Open

fix(fs): exclude ignored file in key listing #622

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
2 changes: 1 addition & 1 deletion src/drivers/utils/node-fs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ export async function readdirRecursive(
files.push(...dirFiles.map((f) => entry.name + "/" + f));
}
} else {
if (!(ignore && ignore(entry.name))) {
if (!(ignore && ignore(entryPath))) {
Copy link
Member

@pi0 pi0 Apr 16, 2025

Choose a reason for hiding this comment

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

I have a feeling some old patterns could only cover entry.name but not sure without testing. to be on safe side, we could check both.

Copy link
Author

@odzhychko odzhychko Apr 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The fix as proposed is indeed breaking for some patterns.

For example, the pattern "ignored.txt" currently would exclude the file "{base}/dir/ignored.txt" from key listings. But after the fix, the pattern would need to be changed to "**/ignored.txt".

One could argue that excluding "{base}/dir/ignored.txt" because of the pattern "ignored.txt" is also a bug (with the same cause as #621) because this pattern does not exclude the file from being watched. The following example shows the pattern having an effect on storage.getKeys but not on storage.watch.

import { createStorage } from "unstorage";
import fsDriver fSo I think, "entry.name" could be checked for backward compatibility. 
But I guess that is something for you as a maintainer to decide. rom "unstorage/drivers/fs";
import { writeFile, mkdir } from "fs/promises";
import { join } from "path";

const testFolders = join("base", "dir");
await mkdir(testFolders, { recursive: true });

const storage = await createStorage({
  driver: fsDriver({
    base: "./base",
    ignore: [
      "ignored.txt"
    ]
  }),
});

const unwatch = await storage.watch((event, key) => {
  console.log(event, key)
});

await writeFile("./base/dir/ignored.txt", "test");
await writeFile("./base/dir/notIgnored.txt", "test");
// "update dir:notIgnored.txt" is logged.
// "update dir:ignored.txt" is not logged.


console.log(await storage.getKeys())
// "[ 'dir:notIgnored.txt' ]"
await unwatch()

Whether what I describe can be considered a bug or just an inconsistent feature worth maintaining backwards compatibility for, is something for you to decide. Please let me know what you decided, and I will make the appropriate change.

My opinion is that backwards compatibility could be maintained without impeding intended usage. Usually, patterns should always start with "**" or "/" because the ignore patterns are applied to an absolute path. But if someone nevertheless just used the file name (e.g., "ignored.txt") and it worked, the current behavior could be maintained (maybe until the next major version) by checking entry.name in addition to entryPath.

files.push(entry.name);
}
}
Expand Down
37 changes: 35 additions & 2 deletions test/drivers/fs.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import { describe, it, expect, vi } from "vitest";
import { describe, it, expect, vi, afterEach } from "vitest";
import { resolve } from "node:path";
import { readFile, writeFile } from "../../src/drivers/utils/node-fs";
import { testDriver } from "./utils";
import { testDriver, type TestContext } from "./utils";
import driver from "../../src/drivers/fs";
import { createStorage } from "../../src";

describe("drivers: fs", () => {
const dir = resolve(__dirname, "tmp/fs");
Expand Down Expand Up @@ -65,4 +66,36 @@ describe("drivers: fs", () => {
});
},
});

const ctx = {} as TestContext;

it("excludes ignored folder in key listing", async () => {
ctx.driver = driver({
base: dir,
ignore: [resolve(dir, "folder1")]
})
ctx.storage = createStorage({
driver: ctx.driver,
})
await ctx.storage.setItem("folder1/file1", "boop");
expect(await ctx.storage.getKeys()).toHaveLength(0)
})

it("excludes ignored file in key listing", async () => {
ctx.driver = driver({
base: dir,
ignore: [resolve(dir, "folder1/file1")]
})
ctx.storage = createStorage({
driver: ctx.driver,
})
await ctx.storage.setItem("folder1/file1", "boop");
expect(await ctx.storage.getKeys()).toHaveLength(0)
})

afterEach(async () => {
await ctx.storage?.clear();
await ctx.storage?.dispose();
await ctx.driver?.dispose?.();
});
});