fs: Replace an Error with a deprecation message.

[ChALkeR]

This fixes a breaking change in commit 353e26e, that was included in a minor version of io.js and broke at least two modules (that were misusing fs.createWriteStream method, but still).

Disclaimer: I am not sure if this is worth fixing, but if it is, this should work.

The two modules I mentioned above were passing a function to fs.createWriteStream, and that was never supported (it was always ignored). In 2.3.0 a more strict options validation was introduced, it started throwing an Error, and that two modules (at least) broke.

[ChALkeR]

Fixes: #1981

[ChALkeR]

Ah, wait a moment. It still doesn't fix this, needs an extra change.

[ChALkeR]

Ok, should be fixed now (for WriteStream).

Though I am not sure if ReadStream needed any changes at all, because it was calling Object.create before, and that should fail if the passed argument is not an object or null.

[thefourtheye]

This message will be printed whenever this situation arises. Is that the intention here? Normally, it is used like this

var warned = false;
...
    warned = internalUtil.printDeprecationMessage('...', warned);

[ChALkeR]

Thanks, fixed.

[thefourtheye]

In the ReadStream block, else is not there. Should we make this consistent?

[ChALkeR]

In the ReadStream, Object.create(options || {}) is called since 4d0329e (v1.5.0). I don't think that there is any need to patch that nowdays and re-introduce pre-1.5.0 behavior.

Actually, even failing with an error on anything but a null in that block (if (options === null || typeof options !== 'object')) should not break anything (compared to v1.5.0). It might be better to add a special case for a null instead of patching that block. What do you think?

[thefourtheye]

Sorry, I meant, options = Object.create(options); used to be outside the if..else if blocks in both the cases. Now it is only inside the else part. Isn't this wrong?

[thefourtheye]

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]

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

[Fishrock123]

I would rather revert, than re-land the original on next (next major) once reviewed.

[ChALkeR]

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

[Fishrock123]

Ack I see now.

I'm not sure if we should throw a deprecation here or just silently fail not do anything like before. cc @nodejs/tsc

[ChALkeR]

It hasn't failed before, the function argument was just ignored. At least for the WriteStream.
And in 2.3.0 it started throwing.

[trevnorris]

can drop this an the other commented type error lines.

[ChALkeR]

Fixed.

[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 closed these PRs (both #1982 and #1998).
If anyone of @nodejs/collaborators thinks that this or #1998 should be reopened or that there needs to be more discussion on this matter, just leave a message here.