fs/promises: ensure options.flag is defaulted to 'r' in readFile #20268

bdistin · 2018-04-24T23:33:00Z

When passing {} or { encoding: 'utf8' } as options to readFile, the
flag is not defaulted to 'r' unlike normal fs. This fix makes
(fs/promises).readFile() act consistently with fs.readFile().

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Trott · 2018-04-24T23:53:27Z

@nodejs/fs

ronkorving · 2018-04-25T01:28:53Z

I'm curious if the behavior of getOptions is as intended. Can someone shed some light on this? Should we not merge these default options into a given object?

BridgeAR · 2018-04-25T10:48:00Z

lib/fs/promises.js

@@ -473,6 +473,7 @@ async function appendFile(path, data, options) {

 async function readFile(path, options) {
  options = getOptions(options, { flag: 'r' });
+  options.flag = options.flag || 'r';


This will actually manipulate the passed in options because getOptions does not copy the options object.
Please do:

if (options.flag == null) { options = copyObject(options); options.flag = 'r'; }

BridgeAR · 2018-04-25T10:48:10Z

@ronkorving you are right in a way: the getOptions changed in a way that breaks with the former behavior. I am looking into that.

bdistin · 2018-04-29T21:48:42Z

No longer manipulates the passed in options. Since options.flag is not needed in readFileHandle(), copied the existing style from writeFile() instead of null check + copyObject().

apapirovski · 2018-05-03T12:39:20Z

@nodejs/fs Please have a look at this PR.

bdistin · 2018-05-09T15:31:52Z

I see that the fs/promises API has been moved to fs.promises, causing conflicts in this pr. Should I resolve those conflicts? (This is my first pr to Node, so I am still a bit unsure of the processes expected.)

trivikr · 2018-05-09T16:20:27Z

@bdistin Yes, please update this PR by following instructions in Step 9

When passing {} or { encoding: 'utf8' } as options to readFile, the flag is not defaulted to 'r' unlike normal fs. This fix makes (fs/promises).readFile() act consistently with fs.readFile().

bdistin · 2018-05-09T16:57:21Z

Also given the changes of the fs/promises paradigm, should the pr/commit be named just fs: instead of fs/promises:?

vsemozhetbyt · 2018-05-09T17:02:03Z

CI: https://ci.nodejs.org/job/node-test-pull-request/14748/

bdistin · 2018-05-14T13:49:07Z

Is there anything more I need to do? I can't explain how the Windows 2016 binary tests fail, and can't see how that would be related to the changes made in this pr. If this is something I need to fix, can anyone shed light on what changes I might need to make?

bzoz · 2018-05-14T15:00:42Z

Windows CI rerun: https://ci.nodejs.org/job/node-test-commit-windows-fanned/17969/

bdistin · 2018-05-16T13:13:36Z

So that CI seemed to have passed, but the checks weren't updated on this pr. Does that mean this is good to go?

bdistin · 2018-05-26T01:51:13Z

bump? @nodejs/fs ??

joyeecheung

I would've expected that getOptions should extend fields of the options if they are not present..but the current fix works as well.

joyeecheung · 2018-05-26T14:55:09Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/15107/

test/parallel/test-fs-promises-readfile-empty.js

+const { promises: fs } = require('fs');
+const fixtures = require('../common/fixtures');
+
+const fn = fixtures.path('empty.txt');


fail on unhandled rejection

fixed

joyeecheung · 2018-05-27T15:35:10Z

CI: https://ci.nodejs.org/job/node-test-pull-request/15117/

common was required, but not assigned to common in order for the requested change to work.

joyeecheung · 2018-05-27T16:44:08Z

CI: https://ci.nodejs.org/job/node-test-pull-request/15118/

bdistin · 2018-05-27T18:16:22Z

It seems with the test fixes, the tests have uncovered another bug: https://github.com/nodejs/node/blob/master/lib/internal/fs/promises.js#L140-L141
Specifically, readFileHandle() ignores the encoding option when the size of the file to read is 0 and returns a buffer instead of an empty string.

How should I handle that bug? Should I increase the scope of this pr to fix that bug as well?

test/parallel/test-fs-promises-readfile-empty.js

+  .then(assert.ok);
+
+fs.readFile(fn, 'utf8')
+  .then(assert.strictEqual.bind(this, ''));


fs: return string on empty fs.promises.readfile when encoding provided fs.promises.readfile() would provide a buffer whenever an empty file is read, when it should have returned an empty string when encoding is provided.

BridgeAR · 2018-05-29T19:51:43Z

CI https://ci.nodejs.org/job/node-test-pull-request/15155/

BridgeAR · 2018-05-30T16:42:08Z

Landed in 2cd3e61 🎉

When passing {} or { encoding: 'utf8' } as options to readFile, the flag is not defaulted to 'r' unlike normal fs. This fix makes fs.promises.readFile() act consistent with fs.readFile(). It also fixes another issue with fs.promises.readfile() where it returned a Buffer instead of an empty string when encoding is provided. PR-URL: nodejs#20268 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ron Korving <ron@ronkorving.nl> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

When passing {} or { encoding: 'utf8' } as options to readFile, the flag is not defaulted to 'r' unlike normal fs. This fix makes fs.promises.readFile() act consistent with fs.readFile(). It also fixes another issue with fs.promises.readfile() where it returned a Buffer instead of an empty string when encoding is provided. PR-URL: #20268 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ron Korving <ron@ronkorving.nl> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

Trott added the fs Issues and PRs related to the fs subsystem / file system. label Apr 24, 2018

vsemozhetbyt added the promises Issues and PRs related to ECMAScript promises. label Apr 24, 2018

bdistin force-pushed the fix-readfile-flag-default branch from 76a8dbb to 58b874d Compare April 25, 2018 01:22

BridgeAR reviewed Apr 25, 2018

View reviewed changes

bdistin force-pushed the fix-readfile-flag-default branch from 58b874d to c64b578 Compare April 29, 2018 21:44

jasnell approved these changes May 3, 2018

View reviewed changes

ronkorving approved these changes May 7, 2018

View reviewed changes

trivikr approved these changes May 7, 2018

View reviewed changes

ChALkeR added the experimental Issues and PRs related to experimental features. label May 8, 2018

bdistin force-pushed the fix-readfile-flag-default branch 4 times, most recently from 3e0d004 to f5a462f Compare May 9, 2018 16:48

fs/promises: ensure options.flag is defaulted to 'r' in readFile

f5a462f

When passing {} or { encoding: 'utf8' } as options to readFile, the flag is not defaulted to 'r' unlike normal fs. This fix makes (fs/promises).readFile() act consistently with fs.readFile().

joyeecheung approved these changes May 26, 2018

View reviewed changes

joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 26, 2018

joyeecheung added dont-land-on-v6.x labels May 26, 2018

joyeecheung previously requested changes May 27, 2018

View reviewed changes

test/parallel/test-fs-promises-readfile-empty.js

const { promises: fs } = require('fs');

const fixtures = require('../common/fixtures');

const fn = fixtures.path('empty.txt');

This comment was marked as resolved.

Sign in to view

joyeecheung removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 27, 2018

test: requested change

26bce85

fail on unhandled rejection

test: fix requested change

ca44fc2

common was required, but not assigned to common in order for the requested change to work.

joyeecheung reviewed May 28, 2018

View reviewed changes

test/parallel/test-fs-promises-readfile-empty.js

.then(assert.ok);

fs.readFile(fn, 'utf8')

.then(assert.strictEqual.bind(this, ''));

This comment was marked as resolved.

Sign in to view

BridgeAR approved these changes May 29, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 29, 2018

BridgeAR closed this May 30, 2018

MylesBorins mentioned this pull request Jun 6, 2018

v10.4.0 proposal #21167

Merged

richardlau mentioned this pull request Jan 10, 2019

fs.promises API is working but why #25435

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs/promises: ensure options.flag is defaulted to 'r' in readFile #20268

fs/promises: ensure options.flag is defaulted to 'r' in readFile #20268

bdistin commented Apr 24, 2018 •

edited

Loading

Trott commented Apr 24, 2018

ronkorving commented Apr 25, 2018

BridgeAR Apr 25, 2018

BridgeAR commented Apr 25, 2018

bdistin commented Apr 29, 2018

apapirovski commented May 3, 2018

bdistin commented May 9, 2018

trivikr commented May 9, 2018

bdistin commented May 9, 2018

vsemozhetbyt commented May 9, 2018

bdistin commented May 14, 2018

bzoz commented May 14, 2018

bdistin commented May 16, 2018

bdistin commented May 26, 2018

joyeecheung left a comment

joyeecheung commented May 26, 2018

This comment was marked as resolved.

joyeecheung commented May 27, 2018

joyeecheung commented May 27, 2018

bdistin commented May 27, 2018 •

edited

Loading

This comment was marked as resolved.

BridgeAR commented May 29, 2018

BridgeAR commented May 30, 2018

fs/promises: ensure options.flag is defaulted to 'r' in readFile #20268

fs/promises: ensure options.flag is defaulted to 'r' in readFile #20268

Conversation

bdistin commented Apr 24, 2018 • edited Loading

Checklist

Trott commented Apr 24, 2018

ronkorving commented Apr 25, 2018

BridgeAR Apr 25, 2018

Choose a reason for hiding this comment

BridgeAR commented Apr 25, 2018

bdistin commented Apr 29, 2018

apapirovski commented May 3, 2018

bdistin commented May 9, 2018

trivikr commented May 9, 2018

bdistin commented May 9, 2018

vsemozhetbyt commented May 9, 2018

bdistin commented May 14, 2018

bzoz commented May 14, 2018

bdistin commented May 16, 2018

bdistin commented May 26, 2018

joyeecheung left a comment

Choose a reason for hiding this comment

joyeecheung commented May 26, 2018

This comment was marked as resolved.

joyeecheung commented May 27, 2018

joyeecheung commented May 27, 2018

bdistin commented May 27, 2018 • edited Loading

This comment was marked as resolved.

BridgeAR commented May 29, 2018

BridgeAR commented May 30, 2018

bdistin commented Apr 24, 2018 •

edited

Loading

bdistin commented May 27, 2018 •

edited

Loading