lib: support dumb terminals by wlodzislav · Pull Request #26261 · nodejs/node (original) (raw)
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation59 Commits8 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Add checks for TERM='dumb'
in console
and repl
.
Add support for dumb terminals in readline
. When TERM='dumb'
:
- it doesn't use any ANSI escape codes for movement and output
- it doesn't use color
- it ignores all special keys except:
escape
,return
andctrl-c
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes- tests are included
- commit message follows commit guidelines
nodejs-github-bot added console
Issues and PRs related to the console subsystem.
Issues and PRs related to the built-in readline module.
Issues and PRs related to the REPL subsystem.
labels
@BridgeAR do we need this? Maybe these cases can be covered via hasColor
from #26247.
It is different from hasColor
. Dumb terminals doesn't support any ANSI escape codes(not only color codes).
According to infocmp dumb
dumb terminals support only escape and return. So TERM=dumb
means that tty couldn't process any escape codes.
readline
uses escape codes to redraw full line every keypress it it's run in tty, regardless of colors.
It is annoying when you run node repl or any cmd with console.log/dir inside GUI versions of VIM or emacs. It makes running node directly from them unusable.
@silverwind as pointed out by @wlodzislav this is indeed required to really support dumb
terminals. The hasColors
function is still useful in this context (otherwise we'd have to change more places throughout the code base).
BridgeAR added the semver-major
PRs that contain breaking changes and should be released in the next major version.
label
this is indeed required to really support dumb terminals
Then I would suggest adding something like tty.supportsEscapes()
. I think its result could be statically determined at startup. Possibly the same for the color methods. Thought that comes with the drawback that users can no longer change the environment vars it uses at runtime.
Having separate write methods _ttyWriteDumb
seems like nonsense.
Having separate write methods
_ttyWriteDumb
seems like nonsense.
I originally thought to use _normalWrite
, but it doesn't do the job - it ignores all incoming keys.
Dumb terminals couldn't process escape codes, but they could send any keys(like C-c).
And using _ttyWrite
means that all branches will be wrapped in if(dumb)
check to ignore all incoming keys that trigger commands that work with the cursor(basically all of them).
I would suggest adding something like
tty.supportsEscapes()
But what does that function do internally? I guess it would just return process.env.TERM !== 'dumb'
.
Having separate write methods
_ttyWriteDumb
seems like nonsense.
We could theoretically just skip mostly everything and add more code to _ttyWrite()
but that just increases the complexity of an already complex function. Separating the logic seems therefore right to me.
- Removed
_ttyWriteDumb
from prototype + replace_ttyWrite
in constructor - Removed Buffer check
- Added
s
string check + check ifs
not empty - Removed unnecessary check for
useColors
inrepl
Adding only typeof
check makes parallel/test-repl-editor.js
fail. When _ttyWrite
is called with s=''
there is an unnescessary call to this._writeToOutput(s)
without non-empty check.
wlodzislav changed the title
Fix: Nodejs STILL sends ANSI escape sequences to dumb terminals. #26187 Fix: Nodejs STILL sends ANSI escape sequences to dumb terminals
But what does that function do internally? I guess it would just return process.env.TERM !== 'dumb'.
Oh, I was under the assumption that is ecompasses more than just that, but I guess if it is that simple, it might make sense to not move it to a method.
Separating the logic seems therefore right to me.
On second thought, it probably makes sense, if we can avoid too much code duplication.
BridgeAR added a commit to BridgeAR/node that referenced this pull request
PR-URL: nodejs#26264 Refs: nodejs#26261 Reviewed-By: Anna Henningsen anna@addaleax.net Reviewed-By: Jeremiah Senkpiel fishrock123@rocketmail.com Reviewed-By: James M Snell jasnell@gmail.com
BridgeAR added a commit to BridgeAR/node that referenced this pull request
- Using
process.env.TERM = 'dumb'
should never return any colors. process.env.TERM = 'terminator'
supports 24 bit colors.- Add support for
process.env.TERM = 'rxvt-unicode-24bit'
Hyper
does not support true colors anymore. It should fall back to the xterm settings in regular cases.process.env.COLORTERM = 'truecolor'
should return 24 bit colors.
PR-URL: nodejs#26264 Refs: nodejs#26261 Reviewed-By: Anna Henningsen anna@addaleax.net Reviewed-By: Jeremiah Senkpiel fishrock123@rocketmail.com Reviewed-By: James M Snell jasnell@gmail.com
BridgeAR added a commit that referenced this pull request
PR-URL: #26264 Refs: #26261 Reviewed-By: Anna Henningsen anna@addaleax.net Reviewed-By: Jeremiah Senkpiel fishrock123@rocketmail.com Reviewed-By: James M Snell jasnell@gmail.com
BridgeAR added a commit that referenced this pull request
- Using
process.env.TERM = 'dumb'
should never return any colors. process.env.TERM = 'terminator'
supports 24 bit colors.- Add support for
process.env.TERM = 'rxvt-unicode-24bit'
Hyper
does not support true colors anymore. It should fall back to the xterm settings in regular cases.process.env.COLORTERM = 'truecolor'
should return 24 bit colors.
PR-URL: #26264 Refs: #26261 Reviewed-By: Anna Henningsen anna@addaleax.net Reviewed-By: Jeremiah Senkpiel fishrock123@rocketmail.com Reviewed-By: James M Snell jasnell@gmail.com
Ping @wlodzislav this needs a rebase and I landed my upstream fix to improve the color detection. So now it should be possible to address #26261 (comment) and #26261 (comment).
This is already in a great state and to me it seems it only needs a little bit more polishing.
When TERM=dumb and .isTTY=true don't use ANSI escape codes and ignore all keys, except 'escape', 'return' and 'ctrl-c'.
Fixes: nodejs#26187
Is there a way to override color depth to ensure colors are outputted even if the terminal suggests it does not output color?
@Fishrock123 see #26485.
This PR is somewhat independent of that though. A dumb
terminal does not only not support colors, it is not capable of handling control sequences either.
@Trott @nodejs/tsc is the TSC fine with landing this as semver-minor instead of semver-major?
I'm not super keen on landing as a minor, because it's not. I don't feel strongly enough to argue it out but my default would be to stick with semver-major. A hypothetical situation is that someone is depending on parsing output in a particular way and they happen to have dumb
set but don't know it, then suddenly the output is dramatically changed with an minor upgrade. I don't know the last time I saw dumb
, though, but it's certainly plausable.
@rvagg in that case this PR only requires two LGs from the @nodejs/tsc.
@nodejs/tsc PTAL. This PR has a couple LGs but it misses one more TSC LG because it's semver-major.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea as a major seems fine to me - I'd like one code clarification though.
options.useColors = self.terminal; |
---|
options.useColors = self.terminal && ( |
typeof self.outputStream.getColorDepth === 'function' ? |
self.outputStream.getColorDepth() > 2 : true); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be > 1
?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch while it won't have any actual impact (the return values are either 1
, 4
, 8
or 24
).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got that check from lib/internal/console/constructor.js:256-258
, should I change the check there too in this PR?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wlodzislav I think it’s okay to leave that for a separate PR, but here it would be good to replace it with > 1
before this is being merged.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed this while landing.
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request
PR-URL: nodejs#26261 Fixes: nodejs#26187 Reviewed-By: Ruben Bridgewater ruben@bridgewater.de Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Roman Reiss me@silverwind.io Reviewed-By: Rich Trott rtrott@gmail.com Reviewed-By: Jeremiah Senkpiel fishrock123@rocketmail.com
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request
When TERM=dumb and .isTTY=true don't use ANSI escape codes and ignore all keys, except 'escape', 'return' and 'ctrl-c'.
PR-URL: nodejs#26261 Fixes: nodejs#26187 Reviewed-By: Ruben Bridgewater ruben@bridgewater.de Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Roman Reiss me@silverwind.io Reviewed-By: Rich Trott rtrott@gmail.com Reviewed-By: Jeremiah Senkpiel fishrock123@rocketmail.com
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request
PR-URL: nodejs#26261 Fixes: nodejs#26187 Reviewed-By: Ruben Bridgewater ruben@bridgewater.de Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Roman Reiss me@silverwind.io Reviewed-By: Rich Trott rtrott@gmail.com Reviewed-By: Jeremiah Senkpiel fishrock123@rocketmail.com
BethGriggs added a commit that referenced this pull request
Notable changes:
- assert:
- async_hooks:
- bootstrap
- make Buffer and process non-enumerable (Ruben Bridgewater) #24874
- buffer:
- child_process:
- console:
- don't use ANSI escape codes when TERM=dumb (Vladislav Kaminsky) #26261
- crypto:
- deps:
- silence irrelevant V8 warning (Michaël Zasso) #26685
- update postmortem metadata generation script (cjihrig) #26685
- V8: un-cherry-pick bd019bd (Refael Ackermann) #26685
- V8: cherry-pick 6 commits (Michaël Zasso) #26685
- V8: cherry-pick d82c9af (Anna Henningsen) #26685
- V8: cherry-pick e5f01ba (Anna Henningsen) #26685
- V8: cherry-pick d5f08e4 (Anna Henningsen) #26685
- V8: cherry-pick 6b09d21 (Anna Henningsen) #26685
- V8: cherry-pick f0bb5d2 (Anna Henningsen) #26685
- V8: cherry-pick 5b0510d (Anna Henningsen) #26685
- V8: cherry-pick 91f0cd0 (Anna Henningsen) #26685
- V8: cherry-pick 392316d (Anna Henningsen) #26685
- V8: cherry-pick 2f79d68 (Anna Henningsen) #26685
- sync V8 gypfiles with 7.4 (Ujjwal Sharma) #26685
- update V8 to 7.4.288.13 (Ujjwal Sharma) #26685
- bump minimum icu version to 63 (Ujjwal Sharma) #25852
- silence irrelevant V8 warnings (Michaël Zasso) #25852
- V8: cherry-pick 7803fa6 (Jon Kunkee) #25852
- V8: cherry-pick 58cefed (Jon Kunkee) #25852
- V8: cherry-pick d3308d0 (Michaël Zasso) #25852
- V8: cherry-pick 74571c8 (Michaël Zasso) #25852
- cherry-pick fc0ddf5 from upstream V8 (Anna Henningsen) #25852
- sync V8 gypfiles with 7.3 (Ujjwal Sharma) #25852
- sync V8 gypfiles with 7.2 (Michaël Zasso) #25852
- update V8 to 7.3.492.25 (Michaël Zasso) #25852
- add s390 asm rules for OpenSSL-1.1.1 (Shigeki Ohtsu) #19794
- sync V8 gypfiles with 7.1 (Refael Ackermann) #23423
- update V8 to 7.1.302.28 (Michaël Zasso) #23423
- doc:
- errors:
- update error name (Ruben Bridgewater) #26738
- fs:
- http:
- lib:
- move DEP0021 to end of life (cjihrig) #27127
- remove Atomics.wake (Gus Caplan) #27033
- validate Error.captureStackTrace() calls (Ruben Bridgewater) #26738
- refactor Error.captureStackTrace() usage (Ruben Bridgewater) #26738
- move DTRACE_* probes out of global scope (James M Snell) #26541
- deprecate _stream_wrap (Sam Roberts) [#26245] (#26245)
- don't use
util.inspect()
internals (Ruben Bridgewater) #24971 - improve error message for MODULE_NOT_FOUND (Ali Ijaz Sheikh) #25690
- requireStack property for MODULE_NOT_FOUND (Ali Ijaz Sheikh) #25690
- move DEP0029 to end of life (cjihrig) #25377
- move DEP0028 to end of life (cjihrig) #25377
- move DEP0027 to end of life (cjihrig) #25377
- move DEP0026 to end of life (cjihrig) #25377
- move DEP0023 to end of life (cjihrig) #25280
- move DEP0006 to end of life (cjihrig) #25279
- remove unintended access to deps/ (Anna Henningsen) #25138
- move DEP0120 to end of life (cjihrig) #24862
- use ES6 class inheritance style (Ruben Bridgewater) #24755
- remove
inherits()
usage (Ruben Bridgewater) #24755
- module:
- n-api:
- remove code from error name (Ruben Bridgewater) #26738
- net:
- net,http2:
- merge setTimeout code (ZYSzys) #25084
- os:
- implement os.type() using uv_os_uname() (cjihrig) #25659
- process:
- readline:
- support TERM=dumb (Vladislav Kaminsky) #26261
- repl:
- src:
- remove unused INT_MAX constant (Sam Roberts) #27078
- update NODE_MODULE_VERSION to 72 (Ujjwal Sharma) #26685
- remove
AddPromiseHook()
(Anna Henningsen) #26574 - update NODE_MODULE_VERSION to 71 (Michaël Zasso) #25852
- clean up MultiIsolatePlatform interface (Anna Henningsen) #26384
- properly configure default heap limits (Ali Ijaz Sheikh) #25576
- remove icuDataDir from node config (GauthamBanasandra) #24780
- explicitly allow JS in ReadHostObject (Yang Guo) #23423
- update postmortem constant (cjihrig) #23423
- update NODE_MODULE_VERSION to 68 (Michaël Zasso) #23423
- tls:
- support TLSv1.3 (Sam Roberts) #26209
- return correct version from getCipher() (Sam Roberts) #26625
- check arg types of renegotiate() (Sam Roberts) #25876
- add code for ERR_TLS_INVALID_PROTOCOL_METHOD (Sam Roberts) #24729
- emit a warning when servername is an IP address (Rodger Combs) #23329
- disable TLS v1.0 and v1.1 by default (Ben Noordhuis) #23814
- remove unused arg to createSecureContext() (Sam Roberts) #24241
- deprecate Server.prototype.setOptions() (cjihrig) #23820
- load NODE_EXTRA_CA_CERTS at startup (Ouyang Yadong) #23354
- util:
- change inspect compact and breakLength default (Ruben Bridgewater) #27109
- improve inspect edge cases (Ruben Bridgewater) #27109
- only the first line of the error message (Simon Zünd) #26685
- don't set the prototype of callbackified functions (Ruben Bridgewater) #26893
- rename callbackified function (Ruben Bridgewater) #26893
- increase function length when using
callbackify()
(Ruben Bridgewater) #26893 - prevent tampering with internals in
inspect()
(Ruben Bridgewater) #26577 - fix proxy inspection (Ruben Bridgewater) #26241
- prevent leaking internal properties (Ruben Bridgewater) #24971
- protect against monkeypatched Object prototype for inspect() (Rich Trott) #25953
- treat format arguments equally (Roman Reiss) #23162
- win, fs:
- detect if symlink target is a directory (Bartosz Sosnowski) #23724
- zlib:
PR-URL: #26930
BethGriggs added a commit that referenced this pull request
Notable changes:
- assert:
- async_hooks:
- bootstrap: make Buffer and process non-enumerable (Ruben Bridgewater) #24874
- buffer:
- child_process:
- console:
- don't use ANSI escape codes when
TERM=dumb
(Vladislav Kaminsky) #26261
- don't use ANSI escape codes when
- crypto:
- remove legacy native handles (Tobias Nießen) #27011
- decode missing passphrase errors (Tobias Nießen) #25208
- remove
Cipher.setAuthTag()
andDecipher.getAuthTag()
(Tobias Nießen) #26249 - remove deprecated
crypto._toBuf()
(Tobias Nießen) #25338 - set
DEFAULT\_ENCODING
property to non-enumerable (Antoine du Hamel) #23222
- deps:
- errors:
- update error name (Ruben Bridgewater) #26738
- fs:
- http:
- lib:
- module:
- remove unintended access to deps/ (Anna Henningsen) #25138
- improve error message for MODULE_NOT_FOUND (Ali Ijaz Sheikh) #25690
- requireStack property for MODULE_NOT_FOUND (Ali Ijaz Sheikh) #25690
- remove dead code (Ruben Bridgewater) #26983
- make
require('.')
never resolve outside the current directory (Ruben Bridgewater) #26973 - throw an error for invalid package.json main entries (Ruben Bridgewater) #26823
- don't search in
require.resolve.paths
(cjihrig) #23683
- net:
- os:
- process:
- readline:
- support TERM=dumb (Vladislav Kaminsky) #26261
- repl:
- src:
- remove unused
INT_MAX
constant (Sam Roberts) #27078 - update
NODE_MODULE_VERSION
to 72 (Ujjwal Sharma) #26685 - remove
AddPromiseHook()
(Anna Henningsen) #26574 - clean up
MultiIsolatePlatform
interface (Anna Henningsen) #26384 - properly configure default heap limits (Ali Ijaz Sheikh) #25576
- remove
icuDataDir
from node config (GauthamBanasandra) #24780
- remove unused
- tls:
- support TLSv1.3 (Sam Roberts) #26209
- return correct version from
getCipher()
(Sam Roberts) #26625 - check arg types of renegotiate() (Sam Roberts) #25876
- add code for
ERR_TLS_INVALID_PROTOCOL_METHOD
(Sam Roberts) #24729 - emit a warning when servername is an IP address (Rodger Combs) #23329
- disable TLS v1.0 and v1.1 by default (Ben Noordhuis) #23814
- remove unused arg to createSecureContext() (Sam Roberts) #24241
- deprecate
Server.prototype.setOptions()
(cjihrig) #23820 - load
NODE_EXTRA_CA_CERTS
at startup (Ouyang Yadong) #23354
- util:
- remove
util.print()
,util.puts()
,util.debug()
andutil.error()
(cjihrig) #25377 - change inspect compact and breakLength default (Ruben Bridgewater) #27109
- improve inspect edge cases (Ruben Bridgewater) #27109
- only the first line of the error message (Simon Zünd) #26685
- don't set the prototype of callbackified functions (Ruben Bridgewater) #26893
- rename callbackified function (Ruben Bridgewater) #26893
- increase function length when using
callbackify()
(Ruben Bridgewater) #26893 - prevent tampering with internals in
inspect()
(Ruben Bridgewater) #26577 - prevent Proxy traps being triggered by
.inspect()
(Ruben Bridgewater) #26241 - prevent leaking internal properties (Ruben Bridgewater) #24971
- protect against monkeypatched Object prototype for inspect() (Rich Trott) #25953
- treat format arguments equally (Roman Reiss) #23162
- remove
- win, fs:
- detect if symlink target is a directory (Bartosz Sosnowski) #23724
- zlib:
PR-URL: #26930
BethGriggs added a commit that referenced this pull request
Notable changes:
- assert:
- async_hooks:
- bootstrap: make Buffer and process non-enumerable (Ruben Bridgewater) #24874
- buffer:
- child_process:
- console:
- don't use ANSI escape codes when
TERM=dumb
(Vladislav Kaminsky) #26261
- don't use ANSI escape codes when
- crypto:
- remove legacy native handles (Tobias Nießen) #27011
- decode missing passphrase errors (Tobias Nießen) #25208
- remove
Cipher.setAuthTag()
andDecipher.getAuthTag()
(Tobias Nießen) #26249 - remove deprecated
crypto._toBuf()
(Tobias Nießen) #25338 - set
DEFAULT\_ENCODING
property to non-enumerable (Antoine du Hamel) #23222
- deps:
- errors:
- update error name (Ruben Bridgewater) #26738
- fs:
- http:
- lib:
- module:
- remove unintended access to deps/ (Anna Henningsen) #25138
- improve error message for MODULE_NOT_FOUND (Ali Ijaz Sheikh) #25690
- requireStack property for MODULE_NOT_FOUND (Ali Ijaz Sheikh) #25690
- remove dead code (Ruben Bridgewater) #26983
- make
require('.')
never resolve outside the current directory (Ruben Bridgewater) #26973 - throw an error for invalid package.json main entries (Ruben Bridgewater) #26823
- don't search in
require.resolve.paths
(cjihrig) #23683
- net:
- os:
- process:
- readline:
- support TERM=dumb (Vladislav Kaminsky) #26261
- repl:
- src:
- remove unused
INT_MAX
constant (Sam Roberts) #27078 - update
NODE_MODULE_VERSION
to 72 (Ujjwal Sharma) #26685 - remove
AddPromiseHook()
(Anna Henningsen) #26574 - clean up
MultiIsolatePlatform
interface (Anna Henningsen) #26384 - properly configure default heap limits (Ali Ijaz Sheikh) #25576
- remove
icuDataDir
from node config (GauthamBanasandra) #24780
- remove unused
- tls:
- support TLSv1.3 (Sam Roberts) #26209
- return correct version from
getCipher()
(Sam Roberts) #26625 - check arg types of renegotiate() (Sam Roberts) #25876
- add code for
ERR_TLS_INVALID_PROTOCOL_METHOD
(Sam Roberts) #24729 - emit a warning when servername is an IP address (Rodger Combs) #23329
- disable TLS v1.0 and v1.1 by default (Ben Noordhuis) #23814
- remove unused arg to createSecureContext() (Sam Roberts) #24241
- deprecate
Server.prototype.setOptions()
(cjihrig) #23820 - load
NODE_EXTRA_CA_CERTS
at startup (Ouyang Yadong) #23354
- util:
- remove
util.print()
,util.puts()
,util.debug()
andutil.error()
(cjihrig) #25377 - change inspect compact and breakLength default (Ruben Bridgewater) #27109
- improve inspect edge cases (Ruben Bridgewater) #27109
- only the first line of the error message (Simon Zünd) #26685
- don't set the prototype of callbackified functions (Ruben Bridgewater) #26893
- rename callbackified function (Ruben Bridgewater) #26893
- increase function length when using
callbackify()
(Ruben Bridgewater) #26893 - prevent tampering with internals in
inspect()
(Ruben Bridgewater) #26577 - prevent Proxy traps being triggered by
.inspect()
(Ruben Bridgewater) #26241 - prevent leaking internal properties (Ruben Bridgewater) #24971
- protect against monkeypatched Object prototype for inspect() (Rich Trott) #25953
- treat format arguments equally (Roman Reiss) #23162
- remove
- win, fs:
- detect if symlink target is a directory (Bartosz Sosnowski) #23724
- zlib:
PR-URL: #26930
This was referenced
Apr 23, 2019
Reviewers
BridgeAR BridgeAR approved these changes
silverwind silverwind approved these changes
jasnell jasnell approved these changes
Trott Trott approved these changes
Fishrock123 Fishrock123 approved these changes
addaleax Awaiting requested review from addaleax
mcollina Awaiting requested review from mcollina
mhdawson Awaiting requested review from mhdawson
Labels
PRs that have at least one approval, no pending requests for changes, and a CI started.
Issues and PRs related to the console subsystem.
Issues and PRs related to the built-in readline module.
Issues and PRs related to the REPL subsystem.
PRs that contain breaking changes and should be released in the next major version.