-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
get node:fs tests passing part 1 #16270
Conversation
Updated 7:02 PM PT - Jan 14th, 2025
✅ @paperclover, your commit 91048cc has passed in 🧪 try this PR locally: bunx bun-pr 16270 |
note: bun shell keeps uppercase
Co-authored-by: Dylan Conway <[email protected]> Co-authored-by: Michael H <[email protected]>
Co-authored-by: RiskyMH <[email protected]>
@nektro we can't use .call in builtins
… chloe/node-fs-1
src/sys.zig
Outdated
// enough bytes for a path alongside an error message and code. | ||
// format taken from Node.js 'exceptions.cc' | ||
// search keyword: `Local<Value> UVException(Isolate* isolate,` | ||
var message_buf: [bun.MAX_PATH_BYTES + 2048]u8 = undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the buffer pool for this to avoid allocating 66 KB of stack space
src/js/node/fs.promises.ts
Outdated
try { | ||
this[kRef](); | ||
console.log({ fd, buffer, offset, length, position }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.log({ fd, buffer, offset, length, position }); |
this pr contains work towards getting node.js test for
node:fs
to pass.note: this is a draft pr. opening early to get a ci run to see where regressions land
non-nodefs changes:
--expose-internals
. this was the wrong strategy to pass node tests. to migrate, rewrite all uses of internalBinding to process.binding.