-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
fs: document intention and caveats of Buffer API #6020
Conversation
`fs` functions support passing and receiving paths as both strings and Buffers. | ||
The latter is intended for filesystems which support non-UTF-8 filenames (except | ||
NTFS, where the conversion to UTF-8 is done automatically) and is normally not | ||
needed. |
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.
Perhaps,
`fs` functions support passing and receiving paths as both strings
and Buffers. The latter is intended to make it easier to work with
filesystems that allow for non-UTF-8 filenames. For most typical
uses, working with paths as Buffers will be unnecessary.
*Note* that on certain file systems (such as NTFS and HFS+) Node.js
will always encode filenames as UTF-8. On such file systems, passing
non-UTF-8 encoded Buffers to `fs` functions will not work as expected.
Good idea generally LGTM but offered some rewording. |
Actually it's impossible to work with non-UTF-8 filenames using the string API, so this is a bit misleading.
Somewhat misleading too. This is only true on NTFS. On NFS+, Node.js doesn't touch filenames - UTF-8 is enforced by the file system. |
|
As this explicitly targets newcomers – it might be a good idea to mention that NTFS and HFS+ are the Windows and OS X standard file systems, respectively, if that’s not too distracting from the point? |
@jasnell Updated using your suggestions, plus an addition from me. I thought we should make it clear somewhere that the string API is just Buffer API with automatic conversion to and from UTF-8. |
LGTM |
PR-URL: #6020 Reviewed-By: James M Snell <[email protected]>
Landed in cf29b2f |
PR-URL: #6020 Reviewed-By: James M Snell <[email protected]>
PR-URL: #6020 Reviewed-By: James M Snell <[email protected]>
PR-URL: #6020 Reviewed-By: James M Snell <[email protected]>
PR-URL: #6020 Reviewed-By: James M Snell <[email protected]>
@jasnell is this relevant to lts? |
No. This is specific to the new buffer support in the fs module. Not
|
PR-URL: #6020 Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#6020 Reviewed-By: James M Snell <[email protected]>
PR-URL: #6020 Reviewed-By: James M Snell <[email protected]>
Pull Request check-list
Please make sure to review and check all of these items:
Affected core subsystem(s)
fs
Description of change
This should prevent potential confusion of newcomers.