FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fs: Replace an Error with a deprecation message. by ChALkeR · Pull Request #1982 · nodejs/node · GitHub

/ node Public

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
Conversation 27 Commits 1 Checks 0 Files changed
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 .js  (1) All 1 file type selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
30 changes: 24 additions & 6 deletions lib/fs.js
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Original file line number Diff line number Diff 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 Down Expand 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 || {};

chrisdickinson Jun 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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-furukawa Jun 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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.

ChALkeR Jun 17, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I will now make an alternate PR for that.

ChALkeR Jun 17, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

See #1998

rlidwka Jun 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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.

ChALkeR Jun 17, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

// a little bit bigger buffer and water marks by default
options = Object.create(options);
Expand Down Expand 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);

thefourtheye Jun 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

ChALkeR Jun 17, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Writable.call(this, options);

Expand Down
Back | FazBrowse Home | New Git URL