console: check that stderr is writable - #5635
Conversation
Sorry, something went wrong.
Sorry, something went wrong.
There was a problem hiding this comment.
extra newline
Sorry, something went wrong.
There was a problem hiding this comment.
Fixed. Thanks.
Sorry, something went wrong.
|
lgtm (I think) |
Sorry, something went wrong.
|
LGTM |
Sorry, something went wrong.
|
I know I marked it semver-major to be conservative, but in that spirit, because it's semver-major, it would be great to get a little more CTC sign-off on it before landing. @nodejs/ctc |
Sorry, something went wrong.
|
bump @nodejs/collaborators |
Sorry, something went wrong.
There was a problem hiding this comment.
To be more explicit about what's going on, mind putting this in an if else with the above? Since if stderr == stdout there's no need to check stderr.write.
Sorry, something went wrong.
Mind an alternative wording in the first sentence? Something like "Now instead stderr mirrors stdout's check in the constructor." Key change is the explicit reference to the check being in "the constructor". Also, this is a semver-minor change. If this was attempted previously: > var c = new console.Console(process.stdout, {});
> c.log('hi')
hi
> c.error('bye')
TypeError: this._stderr.write is not a function
at Console.warn (console.js:44:16)
Or the alternative is that they hack around and close the fd. Which has poor results: > fs.closeSync(process.stderr.fd);
> console.error('foo');
Error: write after end
at writeAfterEnd (_stream_writable.js:167:12)
So there's no reasonable assumption that existing code would rely on existing behavior. Nothing to worry about. (unless, of course, a bad stream was passed to stderr but was never written to, but have a hard time justifying that we guard that case. if it was the case I'd call this a bug fix b/c it allowed users to write invalid code) |
Sorry, something went wrong.
`Console` constructor checks that `stdout.write()` is a function but does not do an equivalent check for `stderr.write()`. If `stderr` is not specified in the constructor, then `stderr` is set to be `stdout`. However, if `stderr` is specified, but `stderr.write()` is not a function, then an exception is not thrown until `console.error()` is called. This change adds the same check for 'stderr' in the constructor that is there for `stdout`. If `stderr` fails the check, then a `TypeError` is thrown. Took the opportunity to copyedit the `console` doc a little too.
|
@trevnorris I've rebased, fixed things up based on your comments, and force pushed. PTAL |
Sorry, something went wrong.
Sorry, something went wrong.
`Console` constructor checks that `stdout.write()` is a function but does not do an equivalent check for `stderr.write()`. If `stderr` is not specified in the constructor, then `stderr` is set to be `stdout`. However, if `stderr` is specified, but `stderr.write()` is not a function, then an exception is not thrown until `console.error()` is called. This change adds the same check for 'stderr' in the constructor that is there for `stdout`. If `stderr` fails the check, then a `TypeError` is thrown. Took the opportunity to copyedit the `console` doc a little too. PR-URL: nodejs#5635 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
`Console` constructor checks that `stdout.write()` is a function but does not do an equivalent check for `stderr.write()`. If `stderr` is not specified in the constructor, then `stderr` is set to be `stdout`. However, if `stderr` is specified, but `stderr.write()` is not a function, then an exception is not thrown until `console.error()` is called. This change adds the same check for 'stderr' in the constructor that is there for `stdout`. If `stderr` fails the check, then a `TypeError` is thrown. Took the opportunity to copyedit the `console` doc a little too. PR-URL: #5635 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
`Console` constructor checks that `stdout.write()` is a function but does not do an equivalent check for `stderr.write()`. If `stderr` is not specified in the constructor, then `stderr` is set to be `stdout`. However, if `stderr` is specified, but `stderr.write()` is not a function, then an exception is not thrown until `console.error()` is called. This change adds the same check for 'stderr' in the constructor that is there for `stdout`. If `stderr` fails the check, then a `TypeError` is thrown. Took the opportunity to copyedit the `console` doc a little too. PR-URL: #5635 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
`Console` constructor checks that `stdout.write()` is a function but does not do an equivalent check for `stderr.write()`. If `stderr` is not specified in the constructor, then `stderr` is set to be `stdout`. However, if `stderr` is specified, but `stderr.write()` is not a function, then an exception is not thrown until `console.error()` is called. This change adds the same check for 'stderr' in the constructor that is there for `stdout`. If `stderr` fails the check, then a `TypeError` is thrown. Took the opportunity to copyedit the `console` doc a little too. PR-URL: #5635 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org>
this change (including linting)?
test (or a benchmark) included?
existing APIs, or introduces new ones)?
Affected core subsystem(s)
console
Description of change
Console constructor checks that stdout.write() is a function but
does not do an equivalent check for stderr.write(). If stderr is not
specified in the constructor, then stderr is set to be stdout.
However, if stderr is specified, but stderr.write() is not a
function, then an exception is not thrown until console.error() is
called.
This change mirrors the check already there for stdout for stderr.
If stderr fails the check, then a TypeError is thrown.
Quite possibly semver-major on the grounds that code out there could
be dependent on the current behavior.
Took the opportunity to copyedit the console doc a little too.