fs: check argument type on createStream method

[yosuke-furukawa]

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 Errorfs.createReadStream('foo.txt','utf8');fs.js:1619options=Object.create(options||{});^ TypeError: Object prototypemayonlybeanObjectornull: utf8atFunction.create(native)atnewReadStream(fs.js:1619:20)atObject.fs.createReadStream(fs.js:1608:10)atObject.<anonymous>(/Users/yosuke/go/src/github.com/yosuke-furukawa/fork/io.js/test/parallel/test-fs-write-stream-encoding.js:12:30)atModule._compile(module.js:410:26)atObject.Module._extensions..js(module.js:428:10)atModule.load(module.js:335:32)atFunction.Module._load(module.js:290:12)atFunction.Module.runMain(module.js:451:10)atstartup(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]

Please re-review for document and test.

[brendanashworth]

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

[cjihrig]

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

[yosuke-furukawa]

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.

[cjihrig]

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.

[brendanashworth]

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

[cjihrig]

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

[thefourtheye]

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

[yosuke-furukawa]

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]

fixed.

[yosuke-furukawa]

Fixed styles.

[yosuke-furukawa]

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

[yosuke-furukawa]

I will land this.

[yosuke-furukawa]

landed 353e26e
Thank you for your review.

[Fishrock123]

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

[yosuke-furukawa]

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.