denoland / deno

A modern runtime for JavaScript and TypeScript.
https://deno.com
MIT License
93.49k stars 5.18k forks source link

`fs/promises` FileHandle does not exist #19165

Open lucacasonato opened 1 year ago

lucacasonato commented 1 year ago

Deno:

> const fs = await import("node:fs/promises");
undefined
> const file3 = await fs.open("./README.md");
undefined
> file3
3
> file3
3
> file3.read
undefined

Node:

> const fs = await import("fs/promises");
undefined
>   const file3 = await fs.open("./tests/e2e_unit/testdata/file.txt");
undefined
> file3
FileHandle {
  _events: [Object: null prototype] {},
  _eventsCount: 0,
  _maxListeners: undefined,
  close: [Function: close],
  [Symbol(kCapture)]: false,
  [Symbol(kHandle)]: FileHandle {},
  [Symbol(kFd)]: 24,
  [Symbol(kRefs)]: 1,
  [Symbol(kClosePromise)]: null
}
> file3.read
[Function: read]
k-nasa commented 1 year ago

I will work on this

k-nasa commented 1 year ago

Hello,

This is my first time contributing to Deno's OSS and I have a few questions.

Initially, I thought the issue could be resolved by simply adding something to the polyfill. However, I realized that FileHandle is not yet present. I understand that we need to add this and establish a sequence of steps for use with fs/promises and other.

However, I'm currently struggling to understand how Deno achieves Node compatibility. Could you please give me an overview of the architecture regarding this?

Also, I believe this document (https://github.com/denoland/deno/tree/main/ext/node/polyfills) is outdated and may not function adequately in its current state. I felt an update might be necessary concerning this.

I would like to propose working on fixing this issue. I would appreciate any advice or feedback on how best to proceed.

Thank you in advance.

bartlomieju commented 1 year ago

Hey @k-nasa, thanks for interest in contributing. I will write up a longer rundown of things that need to happen to polyfill this API. I'll get back to you later tonight.

k-nasa commented 1 year ago

Thank you for your reply. looking forward to it. @bartlomieju

bartlomieju commented 1 year ago

@k-nasa so I would suggest to start very small - focus on adding FileHandle class an implementing FileHandle.readFile call.

The actual FileHandle class in Node can be seen here: https://github.com/nodejs/node/blob/959142a4652f7b6e90743be874fe9c065ed31d99/lib/internal/fs/promises.js#L132-L247

I suggest to skip the EventEmitterMixin(JSTransferable) part for now and only make it extend EventEmitter class. You can add it in ext/node/polyfills/internal/fs/handle.ts.

Then in ext/node/polyfills/_fs/_fs_readFile.ts in readFile function you will need to start with updating the checks for path variable to check if it is an instance of FileHandle.

Once you get that set up we can think about how to actually implement the API - right now I'm thinking that FileHandle should just call Deno.open internally and store the returned resource ID that we will later use for various operations.

Let me know if that's helpful or do you need more pointers to get started.

k-nasa commented 1 year ago

@bartlomieju Thank you! I think this is enough information to get started. I will try to work on it tonight.

From your explanation, it seems that "node compatibility" is not achieved by using Node internally, but by a TypeScript program that provides the same interface and behaves the same way as Node. Is the Rust language included in ext/node closely related to the Deno runtime?

bartlomieju commented 1 year ago

From your explanation, it seems that "node compatibility" is not achieved by using Node internally, but by a TypeScript program that provides the same interface and behaves the same way as Node.

That's correct, we do not use Node.js internally, but instead provide a set of APIs that are available in Node.js but are implemented internally by Deno - the implementation is different, but the API surface is the same and we try to achieve the same behavior as in Node (though that's not always the case).

Is the Rust language included in ext/node closely related to the Deno runtime?

ext/node is using the same "op system" as the rest of Deno - if you take a look at ext/node/ops and runtime/ops you will see that the functions decorated with #[op] attribute look similar in both places. They do different things but they share a common API for calling from JavaScript to Rust.

k-nasa commented 1 year ago

I am working here I'm working on a task list like this:

k-nasa commented 1 year ago

@bartlomieju I was wondering what is the reason for accepting FileHandle in readFile? I thought it would be better to call ext/node/polyfills/_fs/_fs_readFile.ts from FileHandle when implementing the readFile method in FileHandle.

k-nasa commented 1 year ago

FileHandle has this many methods ref https://nodejs.org/api/fs.html#class-filehandle

FileHandle needs to implement these.