fs: Replace an Error with a deprecation message. #1982
Conversation
|
Fixes: #1981 |
|
Ah, wait a moment. It still doesn't fix this, needs an extra change. |
|
Ok, should be fixed now (for Though I am not sure if |
| throw new TypeError('options must be a string or an object'); | ||
| else if (options === null || typeof options !== 'object') { | ||
| //throw new TypeError('options must be a string or an object'); | ||
| internalUtil.printDeprecationMessage('Passing anything but an object or \ |
There was a problem hiding this comment.
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
force-pushed
the
fs-stream-fix
branch
2 times, most recently
from
950029e
to
ee895ff
Compare
| warnedWriteStreamOptions | ||
| ); | ||
| options = options || {}; | ||
| } else { |
There was a problem hiding this comment.
In the ReadStream block, else is not there. Should we make this consistent?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
| ); | ||
| options = options || {}; | ||
| } else { | ||
| options = Object.create(options); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Can you post a code sample? I think that I'm missing the idea here.
|
I would rather revert, than re-land the original on |
|
@Fishrock123 That would break a feature that was already released in 2.3.0. |
|
Ack I see now. I'm not sure if we should throw a deprecation here or just silently |
|
It hasn't failed before, the function argument was just ignored. At least for the |
| else if (options === null || typeof options !== 'object') | ||
| throw new TypeError('options must be a string or an object'); | ||
| else if (options === null || typeof options !== 'object') { | ||
| //throw new TypeError('options must be a string or an object'); |
There was a problem hiding this comment.
can drop this an the other commented type error lines.
This fixes a breaking change in commit 353e26e.
|
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). |
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.createWriteStreammethod, 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.