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.

Sign up for GitHub

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: getOption to populate passed in default values #11630

Closed
wants to merge 1 commit into from
Closed

fs: getOption to populate passed in default values #11630

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions lib/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ function getOptions(options, defaultOptions) {

if (options.encoding !== 'buffer')
assertEncoding(options.encoding);
return options;
return Object.assign({}, defaultOptions, options);
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you benchmarked this? I thought Object.assign() was still slow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

never done a benchmark. do we have any existing ones to compare it with? or any suggestions how to do a benchmark? Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was suggested to use util.extend, would it be faster ?

Copy link
Contributor

@mscdex mscdex Mar 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, util._extend() would probably be a better choice, at least in core. Object.assign() is probably fine for tests and non-runtime stuff.

Copy link
Contributor

Choose a reason for hiding this comment

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

Related discussion: #7165 (comment)

}

function copyObject(source) {
Expand Down Expand Up @@ -293,7 +293,7 @@ fs.readFile = function(path, options, callback) {
}

binding.open(pathModule._makeLong(path),
stringToFlags(options.flag || 'r'),
stringToFlags(options.flag),
0o666,
req);
};
Expand Down Expand Up @@ -1201,7 +1201,7 @@ function writeAll(fd, isUserFd, buffer, offset, length, position, callback_) {
fs.writeFile = function(path, data, options, callback) {
callback = maybeCallback(arguments[arguments.length - 1]);
options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'w' });
const flag = options.flag || 'w';
const flag = options.flag;

if (isFd(path)) {
writeFd(path, true);
Expand Down Expand Up @@ -1260,7 +1260,7 @@ fs.appendFile = function(path, data, options, callback) {
options = copyObject(options);

// force append behavior when using a supplied file descriptor
if (!options.flag || isFd(path))
if (isFd(path))
options.flag = 'a';

fs.writeFile(path, data, options, callback);
Expand Down Expand Up @@ -1739,7 +1739,7 @@ function ReadStream(path, options) {
return new ReadStream(path, options);

// a little bit bigger buffer and water marks by default
options = copyObject(getOptions(options, {}));
options = copyObject(getOptions(options, { flag: 'r' }));
if (options.highWaterMark === undefined)
options.highWaterMark = 64 * 1024;

Expand Down Expand Up @@ -1903,7 +1903,7 @@ function WriteStream(path, options) {
if (!(this instanceof WriteStream))
return new WriteStream(path, options);

options = copyObject(getOptions(options, {}));
options = copyObject(getOptions(options, {flag: 'w'}));

Writable.call(this, options);

Expand Down
18 changes: 9 additions & 9 deletions test/parallel/test-fs-read-stream-inherit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ let paused = false;
}

{
const file3 = fs.createReadStream(fn, Object.create({encoding: 'utf8'}));
const file3 = fs.createReadStream(fn, Object.assign({}, {encoding: 'utf8'}));
file3.length = 0;
file3.on('data', function(data) {
assert.strictEqual(typeof data, 'string');
Expand All @@ -66,7 +66,7 @@ let paused = false;
}

{
const options = Object.create({bufferSize: 1, start: 1, end: 2});
const options = Object.assign({}, {bufferSize: 1, start: 1, end: 2});
const file4 = fs.createReadStream(rangeFile, options);
assert.strictEqual(file4.start, 1);
assert.strictEqual(file4.end, 2);
Expand All @@ -80,7 +80,7 @@ let paused = false;
}

{
const options = Object.create({bufferSize: 1, start: 1});
const options = Object.assign({}, {bufferSize: 1, start: 1});
const file5 = fs.createReadStream(rangeFile, options);
assert.strictEqual(file5.start, 1);
file5.data = '';
Expand All @@ -94,7 +94,7 @@ let paused = false;

// https://github.com/joyent/node/issues/2320
{
const options = Object.create({bufferSize: 1.23, start: 1});
const options = Object.assign({}, {bufferSize: 1.23, start: 1});
const file6 = fs.createReadStream(rangeFile, options);
assert.strictEqual(file6.start, 1);
file6.data = '';
Expand All @@ -108,12 +108,12 @@ let paused = false;

{
assert.throws(function() {
fs.createReadStream(rangeFile, Object.create({start: 10, end: 2}));
fs.createReadStream(rangeFile, Object.assign({}, {start: 10, end: 2}));
}, /"start" option must be <= "end" option/);
}

{
const options = Object.create({start: 0, end: 0});
const options = Object.assign({}, {start: 0, end: 0});
const stream = fs.createReadStream(rangeFile, options);
assert.strictEqual(stream.start, 0);
assert.strictEqual(stream.end, 0);
Expand All @@ -137,7 +137,7 @@ let paused = false;

{
let file7 =
fs.createReadStream(rangeFile, Object.create({autoClose: false }));
fs.createReadStream(rangeFile, Object.assign({}, {autoClose: false }));
assert.strictEqual(file7.autoClose, false);
file7.on('data', function() {});
file7.on('end', common.mustCall(function() {
Expand All @@ -150,7 +150,7 @@ let paused = false;

function file7Next() {
// This will tell us if the fd is usable again or not.
file7 = fs.createReadStream(null, Object.create({fd: file7.fd, start: 0 }));
file7 = fs.createReadStream(null, Object.assign({}, {fd: file7.fd, start: 0 }));
file7.data = '';
file7.on('data', function(data) {
file7.data += data;
Expand All @@ -167,7 +167,7 @@ let paused = false;

// Just to make sure autoClose won't close the stream because of error.
{
const options = Object.create({fd: 13337, autoClose: false});
const options = Object.assign({}, {fd: 13337, autoClose: false});
const file8 = fs.createReadStream(null, options);
file8.on('data', function() {});
file8.on('error', common.mustCall(function() {}));
Expand Down