process: resize stderr on SIGWINCH #2231
Conversation
|
LGTM. I'd say it's definitely a bug fix. |
|
LGTM and I agree with @cjihrig. |
|
LGTMT |
| process.on('SIGWINCH', function() { | ||
| stdout._refreshSize(); | ||
| }); | ||
| process.on('SIGWINCH', stdout._refreshSize); |
There was a problem hiding this comment.
I'm not sure but it looks like this will fail if _refreshSize needs to access this
Edit: that would explain why it was wrapped in a function.
There was a problem hiding this comment.
Isn't this referring to the stream though? i.e. the stdout or stderr objects?
There was a problem hiding this comment.
@Fishrock123 You might have to bind. stdout._refreshSize.bind(stdout)
EDIT: The actual definition https://github.com/nodejs/io.js/blob/master/lib/tty.js#L76-92 uses this.
There was a problem hiding this comment.
it is supposed to, but in this case you are passing the method to the event emitter, without its context.
Result is the same as:
var obj = { method() { console.log(this) } };
obj.method(); // obj
var method = obj.method;
method(); // undefinedThere was a problem hiding this comment.
Doh. Right. Javascript.
Fishrock123
force-pushed
the
stderr-resize
branch
2 times, most recently
from
e35c4a4
to
1b381e2
Compare
|
@targos fixed, PTAL |
|
LGTM |
Fixes: nodejs#2219 PR-URL: nodejs#2231 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Fishrock123
force-pushed
the
stderr-resize
branch
from
1b381e2
to
bf2cd22
Compare
Fixes: #2219
I'd consider this a patch level thing, but it could possibly be considered semver-minor; does anyone have opinions about that?