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

Jump to bottom

fs: check argument type on createStream method #1845

Closed
wants to merge 1 commit into from
Closed

fs: check argument type on createStream method #1845

wants to merge 1 commit into from

Conversation

yosuke-furukawa
Copy link
Member

separeted #1412

On Node.js v0.12

// illegal option but does not throw an Error, just ignore.
fs.createReadStream('foo.txt', 'utf8');

// illegal option but does not throw an Error, just ignore.
fs.createWriteStream('hoge', 'utf8');

On io.js v1.6.4

// illegal option and throw an Error
fs.createReadStream('foo.txt', 'utf8');

fs.js:1619
  options = Object.create(options || {});
                   ^
TypeError: Object prototype may only be an Object or null: utf8
    at Function.create (native)
    at new ReadStream (fs.js:1619:20)
    at Object.fs.createReadStream (fs.js:1608:10)
    at Object.<anonymous> (/Users/yosuke/go/src/github.com/yosuke-furukawa/fork/io.js/test/parallel/test-fs-write-stream-encoding.js:12:30)
    at Module._compile (module.js:410:26)
    at Object.Module._extensions..js (module.js:428:10)
    at Module.load (module.js:335:32)
    at Function.Module._load (module.js:290:12)
    at Function.Module.runMain (module.js:451:10)
    at startup (node.js:124:18)

// illegal option but does not throw an Error, just ignore.
fs.createWriteStream('hoge', 'utf8');

I added document and fixed test to avoid linter errors.

@yosuke-furukawa
Copy link
Member Author

Please re-review for document and test.

@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label May 30, 2015
brendanashworth
brendanashworth reviewed May 30, 2015
View reviewed changes
lib/fs.js
@@ -1617,8 +1617,15 @@ function ReadStream(path, options) {
if (!(this instanceof ReadStream))
return new ReadStream(path, options);

if (options === undefined)
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps use loose equality here to grab null also? It'll avoid an Object.create(null) call too.

Copy link
Contributor

Choose a reason for hiding this comment

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

It might be better to change the check on line 1624 to else if (options === null || typeof options !== 'object')

Copy link
Member Author

Choose a reason for hiding this comment

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

latest node and io.js accept null on fs.createReadStream.
current pull request does not change the behavior on fs.createReadStream, accepts string and throws proper exception.

if we don't accept null, this api may break the compatibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

Current versions accept null and any other falsey value so this is still a breaking change. If this were to land, it would make sense to throw on null, since this is already a breaking change and null is not a useful value to support here. This needs to be given a lot of consideration, especially after the events of this past weekend.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this already a breaking change (disregarding null)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because fs.createReadStream() currently allows any falsey value (false, NaN, etc.). Under this PR, passing in false, for example, would throw.

Copy link
Contributor

Choose a reason for hiding this comment

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

options = options || {} used to allow other falsy values as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

hm. OK. I will disallow null.

our current spec is to allow object only.
And under this PR, we can use string as an encoding.

So, this function allows object and string. the other types are not allowed.
If we found disallowed type, we should throw TypeError.

But null is unclear type.
I think null and undefined are not easy to use properly for beginner.
So my first impression, we should allow null just like undefined.
But I will follow @cjihrig suggestion. his spec is clear.

yosuke-furukawa
yosuke-furukawa reviewed Jun 1, 2015
View reviewed changes
lib/fs.js
options = {};
else if (typeof options === 'string')
options = { encoding: options };
else if (options === null || typeof options !== 'object')
Copy link
Member Author

Choose a reason for hiding this comment

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

fixed.

@yosuke-furukawa
Copy link
Member Author

Fixed styles.

@yosuke-furukawa yosuke-furukawa added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 2, 2015
@yosuke-furukawa
Copy link
Member Author

jenkins: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/754/

@yosuke-furukawa
Copy link
Member Author

I will land this.

yosuke-furukawa added a commit that referenced this pull request Jun 5, 2015
@yosuke-furukawa
fs: Add string encoding option for Stream method
353e26e 
Add string encoding option for fs.createReadStream and
fs.createWriteStream. and check argument type more strictly

PR-URL: #1845
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@yosuke-furukawa
Copy link
Member Author

landed 353e26e
Thank you for your review.

@rvagg rvagg mentioned this pull request Jun 11, 2015
Closed
@Fishrock123
Copy link
Contributor

@yosuke-furukawa looks like this wasn't properly signed-off on. It's best to wait for an explicit LGTM. :)

@yosuke-furukawa
Copy link
Member Author

hm,,, I got some review and gets LGTM on this #1412

I just separate the pull request. But I should be careful more and more.

@jasnell jasnell mentioned this pull request Aug 17, 2019
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
fs Issues and PRs related to the fs subsystem / file system. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@yosuke-furukawa @Fishrock123 @thefourtheye @cjihrig @brendanashworth @mscdex