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

path: add Buffer support to join #34053

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

kkz13250
Copy link

Internalized the handling of Buffer type in the join function in path.js,

@nodejs-github-bot nodejs-github-bot added the path Issues and PRs related to the path subsystem. label Jun 25, 2020
@addaleax addaleax added duplicate Issues and PRs that are duplicates of other issues or PRs. and removed duplicate Issues and PRs that are duplicates of other issues or PRs. labels Jun 25, 2020
@addaleax
Copy link
Member

Sorry for the duplicate tag, I saw that this was fixing the same issue as #33395 but not that this was a solution inside the path module itself.

Fwiw, I think having docs and tests would be very helpful here for deciding whether we want to go this route (this would be the first time that Buffer support is added to path).

@mscdex
Copy link
Contributor

mscdex commented Jun 25, 2020

I think it's better to solve this at the fs layer instead because that is where we already document that we accept Buffer arguments for path/file names for various functions there. In general it doesn't seem like using Buffers for path/file names is that common anyway (I think it was added awhile back specifically for Windows users?) as many filesystems these days are UTF-8-capable.

@addaleax
Copy link
Member

In general it doesn't seem like using Buffers for path/file names is that common anyway (I think it was added awhile back specifically for Windows users?) as many filesystems these days are UTF-8-capable.

It’s more of the reverse situation: Windows/NTFS uses UTF-16 for filenames, which we expose to our users as UTF-8, so strings work just fine there. However, while you could call filesystems on other OSes “UTF-8-capable”, what that means in reality is that no specific encoding is enforced at all. That’s why we have Buffer support in our filesystem APIs.

(That being said, I don’t have a strong opinion here.)

@addaleax addaleax changed the title fs: readdir fails when libuv returns UV_DIRENT_UNKNOWN and the first … path: add Buffer support to join Jun 25, 2020
@addaleax
Copy link
Member

I’ve renamed the PR to be a bit more accurate, and so it doesn’t get confused with #33395

@bcoe
Copy link
Contributor

bcoe commented Aug 22, 2021

@kkz13250 @addaleax I need buffer support for several of path's methods, to be able to eventually call fs.cp(). Also, it fixes potential bugs with fs.rm().

I've started working on an implementation here first before pulling it into Node.js.

Would love feedback, specifically I want to make sure we handle some of the concerns around UTF-16.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
path Issues and PRs related to the path subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants