Skip to content
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

feat(fs/unstable): add readDirSync #6381

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jbronder
Copy link
Contributor

@jbronder jbronder commented Feb 5, 2025

This PR adds the readDirSync API to @std/fs, which is intended to mirror the Deno.readDirSync function. The PR also adds additional @param and @returns documentation to the readDir API.

Towards #6255 .

@jbronder jbronder requested a review from kt3k as a code owner February 5, 2025 00:48
@github-actions github-actions bot added the fs label Feb 5, 2025
Copy link

codecov bot commented Feb 5, 2025

Codecov Report

Attention: Patch coverage is 28.57143% with 10 lines in your changes missing coverage. Please review.

Project coverage is 95.75%. Comparing base (3b75ee7) to head (49c01ca).

Files with missing lines Patch % Lines
fs/unstable_read_dir.ts 28.57% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6381      +/-   ##
==========================================
- Coverage   96.23%   95.75%   -0.48%     
==========================================
  Files         556      554       -2     
  Lines       42065    37209    -4856     
  Branches     6371     6358      -13     
==========================================
- Hits        40481    35630    -4851     
+ Misses       1544     1533      -11     
- Partials       40       46       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kt3k
Copy link
Member

kt3k commented Feb 5, 2025

CI is failing with Deno v1.x because IteratorObject type is not available in Deno 1.x (which uses TypeScript 5.5. IteratorObject was added in TypeScript 5.6).

One workaround is to use Iterable instead, but that prevents us using iterator helper method on it.

Does anyone have an idea to best handle this situation?

@jbronder
Copy link
Contributor Author

jbronder commented Feb 6, 2025

I've only tried testing it on Deno v1.46.0, the function signatures are indeed different for readDirSync:

// In the 1.x Deno namespace:
function readDirSync(path: string | URL): Iterable<DirEntry>

// In the 2.x Deno namespace:
function readDirSync(path: string | URL): IteratorObject<DirEntry>

I suppose there's no real benefit to having readDirSync use a return type of Iterable<DirEntry> for the @std/fs package for both 1.x and 2.x other than to satisfy the CI, unless that is indeed the goal.

After modifying the return type to Iterable<DirType>, the iterable returned by Deno 1.x readDirSync does allow access to Iterator Helper methods after testing it out in a small script in the project root:

import { readDirSync } from "./fs/unstable_read_dir.ts";

const dir = readDirSync("fs/testdata");
const partialList = dir.take(5).toArray();
console.log("partialList:", partialList);

It probably doesn't help from a TypeScript perspective since the function signature does not semantically convey what the function does because of the return type differences.

I can leave this on the back burner for now and make progress on other APIs and return to it.

@@ -38,3 +41,40 @@ export async function* readDir(path: string | URL): AsyncIterable<DirEntry> {
}
}
}

Copy link
Contributor

@petamoriken petamoriken Feb 6, 2025

Choose a reason for hiding this comment

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

How about defining the type of IteratorObject here?

Suggested change
interface IteratorObject<T> extends Iterable<T> {}

In Deno 1 (TypeScript 5.3 or lower), it should be treated as just a new type definition, and in Deno 2 (TypeScript 5.4 or higher), where IteratorObject type exists, it should be treated as a merge of interface types and nothing should happen.

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

Successfully merging this pull request may close these issues.

3 participants