Skip to content

fs: Replace an Error with a deprecation message.#1982

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

Closed
ChALkeR wants to merge 1 commit into from
Closed

fs: Replace an Error with a deprecation message. #1982

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
30 changes: 24 additions & 6 deletions lib/fs.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -36,6 +36,8 @@ const isWindows = process.platform === 'win32';
const DEBUG = process.env.NODE_DEBUG && /fs/.test(process.env.NODE_DEBUG);
const errnoException = util._errnoException;

const internalUtil = require('internal/util');

function throwOptionsError(options) {
throw new TypeError('Expected options to be either an object or a string, ' +
'but got ' + typeof options + ' instead');
Expand DownExpand Up@@ -1608,6 +1610,8 @@ fs.createReadStream = function(path, options) {
return new ReadStream(path, options);
};

var warnedReadStreamOptions = false;

util.inherits(ReadStream, Readable);
fs.ReadStream = ReadStream;

Expand All@@ -1619,8 +1623,14 @@ function ReadStream(path, options) {
options = {};
else if (typeof options === 'string')
options = { encoding: options };
else if (options === null || typeof options !== 'object')
throw new TypeError('options must be a string or an object');
else if (options === null || typeof options !== 'object') {
warnedReadStreamOptions = internalUtil.printDeprecationMessage(
'Passing anything but an object or a string as the second argument to ' +
'ReadStream is deprecated.',
warnedReadStreamOptions
);
options = options || {};

@chrisdickinsonchrisdickinsonJun 15, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My preference is to accept functions as an options property bag option for the time being, so we can keep the "illegal argument!" warning TypeError. We can then evaluate whether or not how folks expect a callback to work is consistent and worth supporting as an option.

@yosuke-furukawayosuke-furukawaJun 15, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@chrisdickinson +1.
We should know how behavior developers expect when 2nd argument is function.

And deprecation is not so good idea. This is not breaking change, we just followed the documentation.
https://iojs.org/api/fs.html#fs_fs_createreadstream_path_options

As a spec, we accept string and object only, TypeError is correct behavior.

@ChALkeRChALkeRJun 17, 2015

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I will now make an alternate PR for that.

@ChALkeRChALkeRJun 17, 2015

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

See #1998

@rlidwkarlidwkaJun 17, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@chrisdickinson, functions are not supported as option bags in other places (e.g. in readFileSync), so I think they shouldn't be supported here either.

@ChALkeRChALkeRJun 17, 2015

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I could s/deprecated/not supported/, if you want =).

}

// a little bit bigger buffer and water marks by default
options = Object.create(options);
Expand DownExpand Up@@ -1780,6 +1790,8 @@ fs.createWriteStream = function(path, options) {
return new WriteStream(path, options);
};

var warnedWriteStreamOptions = false;

util.inherits(WriteStream, Writable);
fs.WriteStream = WriteStream;
function WriteStream(path, options) {
Expand All@@ -1790,10 +1802,16 @@ function WriteStream(path, options) {
options = {};
else if (typeof options === 'string')
options = { encoding: options };
else if (options === null || typeof options !== 'object')
throw new TypeError('options must be a string or an object');

options = Object.create(options);
else if (options === null || typeof options !== 'object') {
warnedWriteStreamOptions = internalUtil.printDeprecationMessage(
'Passing anything but an object or a string as the second argument to ' +
'WriteStream is deprecated.',
warnedWriteStreamOptions
);
options = options || {};
} else {
options = Object.create(options);

@thefourtheyethefourtheyeJun 15, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Lets say I pass a string as an argument to ReadStream, then the options will have encoding in its prototype. But in the WriteStream, encoding will be on the options object itself. Is this okay?

@ChALkeRChALkeRJun 17, 2015

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Can you post a code sample? I think that I'm missing the idea here.

}

Writable.call(this, options);

Expand Down