Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unable to exit REPL via escape codes when TERM=dumb #29111

Closed
silverwind opened this issue Aug 13, 2019 · 3 comments · Fixed by #29149
Closed

Unable to exit REPL via escape codes when TERM=dumb #29111

silverwind opened this issue Aug 13, 2019 · 3 comments · Fixed by #29149
Labels
repl Issues and PRs related to the REPL subsystem.

Comments

@silverwind
Copy link
Contributor

^C or ^D do not exit the REPL when TERM=dumb is set. Is this expected?

$ TERM=dumb node
Welcome to Node.js v12.8.0.
Type ".help" for more information.
>
[press ^C]
(To exit, press ^C again or ^D or type .exit)
[press ^C]
>
[press ^D]
>
[press ^D]
>.exit
[finally out of it]
@silverwind silverwind added the repl Issues and PRs related to the REPL subsystem. label Aug 13, 2019
@bnoordhuis
Copy link
Member

I don't think it's unexpected: the terminfo entry for 'dumb' basically says it doesn't support anything except ^G (bel).

On the other hand, lib/readline.js has some emulation baked in and teaching it about ^D isn't hard:

diff --git a/lib/readline.js b/lib/readline.js
index b741557a4d..1e7047ce1d 100644
--- a/lib/readline.js
+++ b/lib/readline.js
@@ -825,6 +825,9 @@ function _ttyWriteDumb(s, key) {
     }
   }
 
+  if (key.ctrl && key.name === 'd')
+    this.close();
+
   switch (key.name) {
     case 'return':  // Carriage return, i.e. \r
       this._sawReturnAt = Date.now();

The reason ^C doesn't work is because the first ^C leaves a \x03 character (the C in ^C) in the command buffer and that trips up this logic:

node/lib/repl.js

Lines 633 to 638 in 427e534

const empty = self.line.length === 0;
self.clearLine();
_turnOffEditorMode(self);
const cmd = self[kBufferedCommandSymbol];
if (!(cmd && cmd.length > 0) && empty) {

@cjihrig
Copy link
Contributor

cjihrig commented Aug 14, 2019

This change in _ttyWriteDumb() fixes the Control+C issue. I'm not sure if it's correct.

diff --git a/lib/readline.js b/lib/readline.js
index b741557a4d..0606b63585 100644
--- a/lib/readline.js
+++ b/lib/readline.js
@@ -823,6 +823,8 @@ function _ttyWriteDumb(s, key) {
       // This readline instance is finished
       this.close();
     }
+
+    return;
   }
 
   switch (key.name) {

And, like Ben mentioned, the code doesn't currently support Control+D. It looks like it was never added in the original PR.

@legendecas
Copy link
Member

legendecas commented Aug 14, 2019

Control+D would trigger end of stdin stream in most terminal, which has handled by readline.

cjihrig added a commit to cjihrig/node that referenced this issue Aug 17, 2019
This commit adds support for closing a readline interface
on Control+D when the terminal is dumb.

PR-URL: nodejs#29149
Fixes: nodejs#29111
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this issue Aug 19, 2019
This commit adds support for closing a readline interface
on Control+C when the terminal is dumb.

PR-URL: #29149
Fixes: #29111
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this issue Aug 19, 2019
This commit adds support for closing a readline interface
on Control+D when the terminal is dumb.

PR-URL: #29149
Fixes: #29111
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants