compat(fs): opendir and opendirSync
#2576
Conversation
…om/iuioiua/deno_std into compat(fs)-opendir-and-opendirSync
node/_tools/config.json
Outdated
| @@ -34,6 +34,7 @@ | |||
| "test-fs-append-file.js", | |||
| "test-fs-chmod-mask.js", | |||
| "test-fs-chmod.js", | |||
| "test-fs-opendir.js", | |||
| } | ||
| } | ||
|
|
||
| export const opendirPromise = promisify(opendir) as ( |
There was a problem hiding this comment.
Nice to have the promise version 👍
node/_fs/_fs_opendir.ts
Outdated
|
|
||
| try { | ||
| /** Throws if encoding is invalid */ | ||
| new TextDecoder(options.encoding); |
There was a problem hiding this comment.
I think this needs to be validateEncoding() call, which is defined in node/internal/validators.mjs. You can pass, for example, encoding: "binary" in opendir in node.js, but new TextDecoder("binary") throws.
There was a problem hiding this comment.
Ah, ok. Am I supposed to use path for the data parameter?
There was a problem hiding this comment.
Oops, sorry validateEncoding doesn't look correct for this according to the thrown error message. Probably assertEncoding (in node/internal/fs/utils.mjs) is the right choice here.
|
Please don’t merge yet. I need to check that callbacks can’t be called twice. |
Ok, done, alongside some other improvements. My only remaining question is: is there any issue in using |
Is that necessary for throwing error synchronously (to make it more compatible to node)? If so, that should be ok. |
|
Last note (I think): |
Ah, ok. I missed that point. There seem several things still missing for actually passing |
Ok, done. |
|
Thanks for updating! |
For #802 and denoland/deno#18218. Note: my tests were previously passing fine on my machine but now they're not. I'm not sure what I changed.
Also, I'm not sure how to handle
encodingandbufferSizeoptions in regards to the current implementation ofDir.