fs: check stream option and set encoding on fs.WritableStream

[yosuke-furukawa]

Reviewer: @bnoordhuis

Problem 1

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');

Bug found modules

• workshopper/workshopper-adventure@75aee8b
• TypeError: Object prototype may only be an Object or null: utf8 decaffeinate/decaffeinate#21

Cause

I found this pull request #635 . this PR uses Object.create in createReadStream. Object.create does not accept string.

Of course, the above scripts and modules are weird. we have never written such options in our api document.

BUT I have 2 opinions.

• the error message(TypeError: Object prototype may only be an Object or null: utf8) is not so easy to understand. We should return Bad Arguments like other functions.
• fs.createReadStream and fs.createWriteStream should accept 2nd string argument as an encoding type. our existing functions like fs.readFile, fs.writeFile, have same behavior like here. the behavior misleads some developers.

Fix

• 4969579

Problem2

According to the createWriteStream api docs, encoding option could be found in the default.

But I set the encoding option to fs.createWriteStream('foo.txt',{encoding: 'utf8'}), the encoding option is ignored.

Fix

• 2f418b6

This pull request includes the fix both problem1 and problem2. comments and discussions are welcome.

[cjihrig]

You probably should be more strict with this check. For example, options = 0 would slip through.

[yosuke-furukawa]

Thank you, I will fix it.

[bnoordhuis]

LGTM. No strong opinion on the error message.

[yosuke-furukawa]

@bnoordhuis@cjihrig I fixed!! Thank you for your review.

[cjihrig]

You can probably drop the null check, as it will work fine with Object.create().

[yosuke-furukawa]

Fixed!

[cjihrig]

FWIW, this can be interpreted as a breaking change - something was silently failing before could now throw an exception.

[yosuke-furukawa]

Yes, but currently fs.createReadStream throws an exception if illegal option, fs.createWriteStream does not throw.
IMO that is not breaking change. We just fix the different behavior.

[Fishrock123]

If the "is breaking?" is controversial we can just tag it for semver-major and land it in the upcoming 2.0 anyways.

[yosuke-furukawa]

We already break the compatible in fs.createReadStream .
The PR introduces this issue.
Should we revert it ??

[jonathanong]

oh i didn't even know you could set options as a string. never saw that anywhere. i don't consider this "breaking" because 1) it's not documented and 2) it wasn't tested prior.

LGTM!

[yosuke-furukawa]

Could I land it on master ??

[brendanashworth]

+1 for patch, not major, because #635 got through on a patch - @yosuke-furukawa I think so.

[yosuke-furukawa]

OK i will land this. Thanks for your remind @brendanashworth

[brendanashworth]

Closing in favor of the two new ones.