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.

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: remove unnecessary usage of .hasOwnProperty() #635

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
34 changes: 15 additions & 19 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -1606,21 +1606,20 @@ function ReadStream(path, options) {
return new ReadStream(path, options);

// a little bit bigger buffer and water marks by default
options = util._extend({
highWaterMark: 64 * 1024
}, options || {});
options = Object.create(options || {});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added this because i realized that without it, all my .hasOwnProperty() stuff is futile

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine since ReadableStream doesn't use .hasOwnProperty

if (options.highWaterMark === undefined)
options.highWaterMark = 64 * 1024;

Readable.call(this, options);

this.path = path;
this.fd = options.hasOwnProperty('fd') ? options.fd : null;
this.flags = options.hasOwnProperty('flags') ? options.flags : 'r';
this.mode = options.hasOwnProperty('mode') ? options.mode : 0o666;

this.start = options.hasOwnProperty('start') ? options.start : undefined;
this.end = options.hasOwnProperty('end') ? options.end : undefined;
this.autoClose = options.hasOwnProperty('autoClose') ?
options.autoClose : true;
this.fd = options.fd === undefined ? null : options.fd;
this.flags = options.flags === undefined ? 'r' : options.flags;
this.mode = options.mode === undefined ? 0o666 : options.mode;

this.start = options.start === undefined ? undefined : options.start;
this.end = options.end === undefined ? undefined : options.end;
this.autoClose = options.autoClose === undefined ? true : options.autoClose;
this.pos = undefined;

if (this.start !== undefined) {
Expand Down Expand Up @@ -1777,13 +1776,11 @@ function WriteStream(path, options) {
Writable.call(this, options);

this.path = path;
this.fd = null;

this.fd = options.hasOwnProperty('fd') ? options.fd : null;
this.flags = options.hasOwnProperty('flags') ? options.flags : 'w';
this.mode = options.hasOwnProperty('mode') ? options.mode : 0o666;
this.fd = options.fd === undefined ? null : options.fd;
this.flags = options.flags === undefined ? 'w' : options.flags;
this.mode = options.mode === undefined ? 0o666 : options.mode;

this.start = options.hasOwnProperty('start') ? options.start : undefined;
this.start = options.start === undefined ? undefined : options.start;
this.pos = undefined;
this.bytesWritten = 0;

Expand Down Expand Up @@ -1863,8 +1860,7 @@ function SyncWriteStream(fd, options) {
this.fd = fd;
this.writable = true;
this.readable = false;
this.autoClose = options.hasOwnProperty('autoClose') ?
options.autoClose : true;
this.autoClose = options.autoClose === undefined ? true : options.autoClose;
}

util.inherits(SyncWriteStream, Stream);
Expand Down
180 changes: 180 additions & 0 deletions test/parallel/test-fs-read-stream-inherit.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,180 @@
var common = require('../common');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just copied test-fs-read-stream.js and wrapped all the options in Object.create(). is that okay? or did you want something more unit where i just check stream.fd or something?

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks fine, but since you mentioned maybe it's a good idea to add some assertions that options were applied :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if the options weren't applied, the same tests wouldn't work... no?

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe, but just to be sure

var assert = require('assert');

// TODO Improved this test. test_ca.pem is too small. A proper test would
// great a large utf8 (with multibyte chars) file and stream it in,
// performing sanity checks throughout.

var path = require('path');
var fs = require('fs');
var fn = path.join(common.fixturesDir, 'elipses.txt');
var rangeFile = path.join(common.fixturesDir, 'x.txt');

var callbacks = { open: 0, end: 0, close: 0 };

var paused = false;

var file = fs.ReadStream(fn);

file.on('open', function(fd) {
file.length = 0;
callbacks.open++;
assert.equal('number', typeof fd);
assert.ok(file.readable);

// GH-535
file.pause();
file.resume();
file.pause();
file.resume();
});

file.on('data', function(data) {
assert.ok(data instanceof Buffer);
assert.ok(!paused);
file.length += data.length;

paused = true;
file.pause();

setTimeout(function() {
paused = false;
file.resume();
}, 10);
});


file.on('end', function(chunk) {
callbacks.end++;
});


file.on('close', function() {
callbacks.close++;

//assert.equal(fs.readFileSync(fn), fileContent);
});

var file3 = fs.createReadStream(fn, Object.create({encoding: 'utf8'}));
file3.length = 0;
file3.on('data', function(data) {
assert.equal('string', typeof(data));
file3.length += data.length;

for (var i = 0; i < data.length; i++) {
// http://www.fileformat.info/info/unicode/char/2026/index.htm
assert.equal('\u2026', data[i]);
}
});

file3.on('close', function() {
callbacks.close++;
});

process.on('exit', function() {
assert.equal(1, callbacks.open);
assert.equal(1, callbacks.end);
assert.equal(2, callbacks.close);
assert.equal(30000, file.length);
assert.equal(10000, file3.length);
console.error('ok');
});

var file4 = fs.createReadStream(rangeFile, Object.create({bufferSize: 1, start: 1, end: 2}));
assert.equal(file4.start, 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a few assert.equal()s as requested @vkurchatkin

assert.equal(file4.end, 2);
var contentRead = '';
file4.on('data', function(data) {
contentRead += data.toString('utf-8');
});
file4.on('end', function(data) {
assert.equal(contentRead, 'yz');
});

var file5 = fs.createReadStream(rangeFile, Object.create({bufferSize: 1, start: 1}));
assert.equal(file5.start, 1);
file5.data = '';
file5.on('data', function(data) {
file5.data += data.toString('utf-8');
});
file5.on('end', function() {
assert.equal(file5.data, 'yz\n');
});

// https://github.com/joyent/node/issues/2320
var file6 = fs.createReadStream(rangeFile, Object.create({bufferSize: 1.23, start: 1}));
assert.equal(file6.start, 1);
file6.data = '';
file6.on('data', function(data) {
file6.data += data.toString('utf-8');
});
file6.on('end', function() {
assert.equal(file6.data, 'yz\n');
});

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

var stream = fs.createReadStream(rangeFile, Object.create({ start: 0, end: 0 }));
assert.equal(stream.start, 0);
assert.equal(stream.end, 0);
stream.data = '';

stream.on('data', function(chunk) {
stream.data += chunk;
});

stream.on('end', function() {
assert.equal('x', stream.data);
});

// pause and then resume immediately.
var pauseRes = fs.createReadStream(rangeFile);
pauseRes.pause();
pauseRes.resume();

var file7 = fs.createReadStream(rangeFile, Object.create({autoClose: false }));
assert.equal(file7.autoClose, false);
file7.on('data', function() {});
file7.on('end', function() {
process.nextTick(function() {
assert(!file7.closed);
assert(!file7.destroyed);
file7Next();
});
});

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.data = '';
file7.on('data', function(data) {
file7.data += data;
});
file7.on('end', function(err) {
assert.equal(file7.data, 'xyz\n');
});
}

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

// Make sure stream is destroyed when file does not exist.
var file9 = fs.createReadStream('/path/to/file/that/does/not/exist');
file9.on('data', function() {});
file9.on('error', common.mustCall(function() {}));

process.on('exit', function() {
assert(file7.closed);
assert(file7.destroyed);

assert(!file8.closed);
assert(!file8.destroyed);
assert(file8.fd);

assert(!file9.closed);
assert(file9.destroyed);
});