Skip to content

feat(fs/unstable): add open, openSync, and FsFile class #6524

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

Merged
merged 17 commits into from
Apr 15, 2025

Conversation

jbronder
Copy link
Contributor

@jbronder jbronder commented Mar 27, 2025

This PR adds open and openSync APIs to the @std/fs package which are intended to mirror the Deno.open and Deno.openSync functions. In support of these APIs, the OpenOptions type and getOpenFsFlag function are added to this PR.

Also, since the open(Sync) APIs return an FsFile object, the FsFile interface, NodeFsFile internal class to convert a Node.js file descriptor into an FsFile instance as well as supporting runtime builtin module request functions are also added to this PR, (i.e. getNodeStream, getNodeTty, and getNodeUtil functions). Associated tests of FsFile methods are also added to this PR.

Towards #6255.

Testing of the open(Sync) APIs are predominately intended to test OpenOptions.

Test of the NodeFsFile is intended to test the instance methods of these objects.

Caveats regarding NodeFsFile implementation

The NodeFsFile does not include the following implementations with reasoning for omissions:

  • seek(Sync): These methods rely on having the SeekMode enum. At the time of submitting this PR, Node.js cannot process enum types without enabling the experimental flag --experimental-transform-types in Node. Running Node.js with TS files only strips types for Node.js >= v23.6.0. For the purposes of passing CI, the SeekMode enum and seek(Sync) methods have been omitted.

  • lock(Sync) and unlock(Sync): These methods do not appear to have a direct Node.js equivalent. Based on the comments raised in issues from the Githubnodejs/node repo: #49256, #54446, and #122 a Node.js equivalent may never materialize at all. To briefly summarize comments, the complexity of providing a cross-platform flock implementation is driving the decision to not implement advisory file-locking in Node.js. As a result, a new Error("Method not implemented.") is suggested to be thrown for these methods.

  • setRaw: Calling the Node.js version this method, setRawMode, appears to require instantiating a tty.ReadStream object with the current underlying file descriptor. I'm currently running into EAGAIN (errno 35) errors on testing. It's likely that I need to spend more time on this method. In the meantime, a new Error("Method not implemented.") is provided as a placeholder.

The Disposable interface/protocol has been implemented in the NodeFsFile class for users writing in TypeScript with Node.js such as using npm package like tsx to run programs. Since the using keyword isn't recognized in Node.js yet, test cases have not been developed since Node runs tests in JavaScript and Explicit File Resource Management is currently at Stage 3.

@jbronder jbronder requested a review from kt3k as a code owner March 27, 2025 23:23
@github-actions github-actions bot added the fs label Mar 27, 2025
Copy link

codecov bot commented Mar 27, 2025

Codecov Report

Attention: Patch coverage is 11.00917% with 291 lines in your changes missing coverage. Please review.

Project coverage is 94.59%. Comparing base (b5a5fe4) to head (cf77904).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
fs/_node_fs_file.ts 6.98% 173 Missing ⚠️
fs/_get_fs_flag.ts 1.85% 53 Missing ⚠️
fs/unstable_open.ts 26.38% 53 Missing ⚠️
fs/_utils.ts 20.00% 12 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6524      +/-   ##
==========================================
- Coverage   95.21%   94.59%   -0.63%     
==========================================
  Files         577      579       +2     
  Lines       43381    43708     +327     
  Branches     6487     6489       +2     
==========================================
+ Hits        41307    41344      +37     
- Misses       2034     2324     +290     
  Partials       40       40              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jbronder
Copy link
Contributor Author

I apologize for the PR that I now realize, in hindsight, was probably a large number of lines of code to review in one PR. I should've made the commits much smaller and discussed with you a better commit approach before sending out this PR.

If it helps in your review, @kt3k, I can either stub out the NodeFsFile methods to throw with new Error("Method not implemented.") or omit methods entirely. The goal would be to get the open(Sync) APIs and a minimum implementation of the NodeFsFile class reviewed first. I can then submit forthcoming PRs for NodeFsFile method implementations gradually with the relevant tests over time. Please let me know if there is another approach you'd like to see regarding this PR.

@kt3k
Copy link
Member

kt3k commented Apr 14, 2025

@jbronder Sorry for the delay in review. This PR looks in a good state. No need of splitting in my view. I'm going to review in more details soon.

@kt3k
Copy link
Member

kt3k commented Apr 14, 2025

@jbronder I'll review in more details tomorrow

}

async read(p: Uint8Array): Promise<number | null> {
const nodeReadFd = this.#nodeUtil.promisify(this.#nodeFs.read);
Copy link
Member

Choose a reason for hiding this comment

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

Can we store this function at this.#nodeReadFs in the constructor? It feels like too much overhead to call promisify for each read

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe store in this function only when it's not initialized yet.

These notes were mistakes in upstream. See denoland/deno#28886
Comment on lines +67 to +68
// This error is added to match the Deno runtime. Deno throws a `TypeError`
// (os error 22) for this OpenOption combo. Under Node.js, the bitmask
Copy link
Member

Choose a reason for hiding this comment

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

Looks reasonable. Nice

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

Looks nicely implemented and tested! Thank you for your contribution!

@kt3k kt3k merged commit 79ae885 into denoland:main Apr 15, 2025
18 checks passed
@jbronder
Copy link
Contributor Author

Thank you for looking over my PR and updating the promisified read and write calls. Good call! I appreciate your time reviewing, @kt3k!

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