fs.createWriteStream TypeError with new io.js v2.3.0

[bricss]

karma-runner/karma#1454

[ChALkeR]

Could be related to: 353e26e 8357c50

@yosuke-furukawa

[ChALkeR]

I will test this later today.

[targos]

It is indeed related to 353e26e.

karma-html-reporter is using fs.createWriteStream with a callback, which is not supported and AFAIK has never been. This sudden error is uncovering a bug that needs to be fixed in the module.

[ChALkeR]

True.
https://iojs.org/api/fs.html#fs_fs_createwritestream_path_options says nothing about callbacks, so that's not supported.

[bricss]

Yep, they should fix this bug with close event.

[ChALkeR]

Still, it might be sensible to add a work-around for that on the io.js side.

[ChALkeR]

Some other places:

g/gist-backup-1.0.0.tgz/gist-backup.js:                    request(raw_url).pipe(fs.createWriteStream(dir + '/' + filename, function (error) {
m/mecano-0.4.1.tgz/lib/download.js:            return fs.createWriteStream(null, stageDestination, function(err, ws) {
m/mecano-0.4.1.tgz/lib/download.js:        return fs.createWriteStream(null, stageDestination, function(err, ws) {
m/mecano-0.4.1.tgz/lib/upload.js:        return fs.createWriteStream(options.ssh, options.destination, function(err, ws) {

[targos]

mecano is not using the fs module, but ssh2-fs instead. This library seems to accept a callback in createWriteStream

[ChALkeR]

Ah, ok. And for gist-backup I filed an issue.
Btw, that's not a full modules list.

[targos]

Btw, that's not a full modules list.

Yes I understand that, but anyway a module that is using a callback to check for error is wrong. It should listen for the error event, like with any stream.

[cjihrig]

We may want to revert 353e26e. It should have landed on the next branch, not master. It was also never properly signed off by anyone, even though the commit message says that I did.

[targos]

@cjihrig I agree. Even though it is wrong (and useless) to pass anything other than an object to these functions, it is still a breaking change.

[targos]

But the PR also added this part:

else if (typeof options === 'string')
  options = { encoding: options };

so reverting it would break code relying on this new functionality...

[ChALkeR]

I suggest we could fix that instead.
A simple check on whether the second argument is a function might work.

[rlidwka]

A simple check on whether the second argument is a function might work.

+ a deprecation message saying "hey, it shouldn't be a function after all".

But if there are only 2 modules in npm relying on this, is it really worth it?

[ChALkeR]

@rlidwka, That's not the full list. it's just the list modules that I have found. And I have a single version of less than 1/3 of npm modules, and run grep with a non-generic regexp.

Can we comment out that TypeError in two places and replace it with a deprecation warning (not setting options to {}, it's not needed there) until some next major release?

[Fishrock123]

+1 on revert and land on next once reviewed.

[ChALkeR]

@Fishrock123 That would break a feature that was already released in 2.3.0.

[yosuke-furukawa]

-1 on revert.

If revert this, we got the fs.createReadStream error again. #1412

This does not break compatible. We should check the arguments properly.
If the argument is wrong, we should assert the illegal arguments.

[chrisdickinson]

@yosuke-furukawa brings up a good point – will more code break if we revert back to the original behavior than if we leave the change in? I suppose it boils down to: are more people erroneously passing strings to createReadStream, or are more people erroneously passing functions to createReadStream?

I am leaning towards patching the typeof options check to also allow function, and to treat it as an options object. We might look into what folks expect passing a function should do, also, so we can determine whether it's worth supporting that!

[brendanashworth]

Please allow both rather than only reverting one (and breaking new functionality) or leaving it in (and breaking existing functionality). Since we know it'd break something, it'd be semver-major!

[trevnorris]

Ridiculous solution: if they pass a callback then create a new Error('no passing a callback!') pass that to the callback and return early.

[ChALkeR]

@trevnorris That would still be a breaking change.

[bricss]

Maybe it would be better to just ignore everything except strings and objects as an options?

[yosuke-furukawa]

-1: ignore
-1: revert
+0.5: replace error to deprecation #1982

We should know what is the expected behavior when user calls fs.createWritableStream('example.txt', function(){}).

And we write the specification, we should follow the spec. we should not support the illegal function call.
https://iojs.org/api/fs.html#fs_fs_createreadstream_path_options

If this is breaking change, we already broke fs.createReadStream here. #635 #1412
this PR is landed in 1.5.0, it is not 2.0.0.

[bnoordhuis]

Just my EUR .02 but the broken modules are broken because they are buggy. I don't see any reason to revert the change or add workarounds, just PR the offenders as a courtesy and move on.

[ChALkeR]

@bnoordhuis One module is already fixed, and @targos already filed a PR for the second one.
I don't know if there are more. Also, see #1998.

Edit: mentioned the wrong person, fixed. Sorry.

[cjihrig]

I'd also rather fix the couple broken modules than land #1998 and #1982.

[Fishrock123]

Yeah, I'm not so sure I like either also.

[ChALkeR]

Ok, as no one (including myself) seems to actually support the idea of handling this (wrong API usage by modules) on the io.js side, I am closing both #1982 and #1998 PRs and this issue.

This is a bug in the module that is using the API in an incorrect and unsupported way.

A pull request targeting karma-html-reporter is here: dtabuenc/karma-html-reporter#27 (thanks, @targos), karma-html-reporter should merge it to solve its problem.

If anyone of @nodejs/collaborators thinks that this should be reopened or that there needs to be more discussion on this matter, just leave a message here.

[yosuke-furukawa]

@ChALkeR @bnoordhuis @cjihrig
Thank you soooo much. I agreed bnoordhuis and cjihrig's idea.