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.

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: main
Choose a base branch
from

Conversation

odzhychko
Copy link

The ignore pattern was applied only to the file name and not to the full file path.
This caused files that should have been ignored to appear in the key listing.

Resolves #621

The ignore pattern was applied only to the file name and not to the full file path.
This caused files that should have been ignored to appear in the key listing.

Resolves unjs#621
@odzhychko odzhychko requested a review from pi0 as a code owner April 15, 2025 19:24
@@ -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

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ignored files are not excluded from key listing
2 participants