Skip to content

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Apr 6, 2022

Backport of #41677.

/cc @juanarbol in case it's still time to pull it to v17.9.0.

The FS docs wrongfully indicated support for passing object with an own
`toString` function property to `FileHandle.prototype.appendFile`,
`FileHandle.prototype.writeFile`, `FileHandle.prototype.write`,
`fsPromises.writeFile`, and `fs.writeSync`. This commit fixes that, and
adds some test to ensure the actual behavior is aligned with the docs.
It also fixes a bug that makes the process crash if a non-buffer object
was passed to `FileHandle.prototype.write`.

Refs: nodejs#34993
PR-URL: nodejs#41677
Refs: nodejs#41666
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. v17.x labels Apr 6, 2022
Copy link
Member

@juanarbol juanarbol left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 7, 2022

@nodejs-github-bot

This comment was marked as outdated.

juanarbol pushed a commit that referenced this pull request Apr 7, 2022
The FS docs wrongfully indicated support for passing object with an own
`toString` function property to `FileHandle.prototype.appendFile`,
`FileHandle.prototype.writeFile`, `FileHandle.prototype.write`,
`fsPromises.writeFile`, and `fs.writeSync`. This commit fixes that, and
adds some test to ensure the actual behavior is aligned with the docs.
It also fixes a bug that makes the process crash if a non-buffer object
was passed to `FileHandle.prototype.write`.

Refs: #34993
PR-URL: #41677
Backport-PR-URL: #42631
Refs: #41666
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@juanarbol
Copy link
Member

Landed in 8009cb0 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants