fs: do not pass Buffer when toString() fails#9670
Uh oh!
There was an error while loading. Please reload this page.
Conversation
mscdex added fs 
nodejs-github-bot added the fs 
sam-github left a comment
sam-github left a comment There was a problem hiding this comment.
Isn't the bufer useful? Particularly here, converting the Buffer to a string failed, but the buffer was sucessfully read, so why not return the data?
lib/fs.js Outdated
There was a problem hiding this comment.
Shouldn't this move to line 401, before the buffer object (which is no longer being passed), goes through the code to create/change it?
mscdexforce-pushed the fs-tostring-failure-callback branch from 733e8e6 to 7b18106Compare
@sam-github I think it's more confusing not only because we don't do that for any other node core API (that I am aware of) but because if you asked for a string, I would never expect a Buffer -- whether there was an error or not. Also, the Buffer is probably useless in most cases since there is usually a reason why a string was requested. |
mscdex commented Nov 18, 2016
EDIT: CI is green |
mscdexforce-pushed the fs-tostring-failure-callback branch from 7b18106 to dc918d7Compare
Even though an Error object is passed to the callback when readFile() fails due to toString() failing, it is a bit strange to still see data passed as the second argument. This commit changes that and only passes the Error object in that case. PR-URL: nodejs#9670 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Prince John Wesley <[email protected]> Reviewed-By: James M Snell <[email protected]>
mscdexforce-pushed the fs-tostring-failure-callback branch from dc918d7 to f3cf8e9CompareEven though an Error object is passed to the callback when readFile() fails due to toString() failing, it is a bit strange to still see data passed as the second argument. This commit changes that and only passes the Error object in that case. PR-URL: nodejs#9670 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Prince John Wesley <[email protected]> Reviewed-By: James M Snell <[email protected]>
8 participants
Checklist
make -j8 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
Description of change
Even though an Error object is passed to the callback when
readFile()fails due totoString()failing, it is a bit strange to still see data passed as the second argument. This commit changes that and onlypasses the Error object in that case.
CI: https://ci.nodejs.org/job/node-test-pull-request/4877/