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

Node compat: Add path member to fs.Dirent #7649

Closed
wants to merge 16 commits into from

Conversation

NReilingh
Copy link
Contributor

@NReilingh NReilingh commented Dec 14, 2023

What does this PR do?

This changes the output of fs.readdir({ withFileTypes: true }) (and all of its variants) to be consistent with node, such that the Dirent.name property is the path basename and the Dirent.path property is the path dirname. Fixes #7358. This is a "breaking" change since the Dirent.name property currently contains the full relative path for { recursive: true }.

  • Documentation or TypeScript types (it's okay to leave the rest blank in this case)
  • Code changes

How did you verify your code works?

I wrote a script at test/dirent.js to manually compare the behavior of node and bun for a mock set of folders, subfolders, and files addressed by different paths. I'm new to this repo and to Zig so I know this isn't idiomatic, but it's the best I could come up with so far.

  • I checked the lifetime of memory allocated to verify it's (1) freed and (2) only freed when it should be
  • I included a test for the new code, or an existing test covers it
  • JSValue used outside outside of the stack is either wrapped in a JSC.Strong or is JSValueProtect'ed
  • I wrote TypeScript/JavaScript tests and they pass locally (bun-debug test test-file-name.test)

I'm coming in here with very little systems programming experience, so I did my best to follow any patterns I could find that appeared to be relevant to my changes. For example, I added item.path.deref() anywhere I saw item.name.deref().

  • I added TypeScript types for the new methods, getters, or setters

@since parameter on the TS type needs to be populated if/when this is scheduled to a release.

None of my changes seem to affect the behavior of bun repl.

three locations in node_fs.zig are apparently where code changes are needed. Should also check to see if there are tests to add.
This is not the right place for this
I believe `path` is always the passed-in relative or absolute path in this case
To test all three of Bun's withFileTypes implementations
test/dirent.js Outdated
// This script tests the contents of fs.Dirent for all of the methods that return it.
// Compare running with node vs. running with Bun for compatibility.

const fs = require('fs');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add automated tests

This is not an automated test. It won't run in CI.

Have a look at files with .test.js or .test.ts for an exmaple

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I now see the readdir tests in test/js/node/fs/fs.test.ts -- is it preferred to add these tests to that same file?

Copy link
Member

Choose a reason for hiding this comment

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

that sounds like a good place for the test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some passing tests to test/js/node/fs/fs.test.ts. Ideally I would also test relative paths (i.e. readdir('../..')) for behavior consistent with node, but I'm not sure if it's worthwhile to do so, or what a good approach would be for setting the test's working directory.

Copy link
Member

@paperclover paperclover left a comment

Choose a reason for hiding this comment

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

overall looks fine

however path is deprecated in node and they intend to replace it with parentPath

nodejs/node#50976
nodejs/node#51020

we should add both properties, where one is an alias for the other.

@NReilingh
Copy link
Contributor Author

@paperdave Ah, didn't know about that Node.js deprecation. I made a change so that both path and parentPath work.

/**
* The base path that this `fs.Dirent` object refers to.
* Deprecated in favor of `parentPath`.
* @since v??????
Copy link
Member

@paperclover paperclover Jan 5, 2024

Choose a reason for hiding this comment

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

since ???? should be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like this entire file gets deleted to resolve the merge conflict -- if I'm understanding things correctly, type definitions are now in https://github.com/DefinitelyTyped/DefinitelyTyped per #7670.

Copy link
Member

@paperclover paperclover left a comment

Choose a reason for hiding this comment

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

i can update the comment in the code that needs to be changed. looks to go me otherwise

@paperclover paperclover self-assigned this Jan 5, 2024
Copy link
Collaborator

@Jarred-Sumner Jarred-Sumner left a comment

Choose a reason for hiding this comment

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

I don't think this should've been approved

We should use a rope string for the path getter. The path getter will rarely be called. As-is, this will be a significant memory usage increase to readdir, which matters when calling it on a large directory tree

To create a rope string, we can use jsRopeString in C++. We would need to add a binding to BunString.cpp that creates a rope string from two different BunString pointers and increment the reference count for the path string.

@paperclover paperclover removed their assignment Jan 8, 2024
@NReilingh
Copy link
Contributor Author

Closing because a different implementation was merged: #11135

@NReilingh NReilingh closed this May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node compat: fs.Dirent does not have path member
3 participants