fix: possible fix for isTTY and GitBash #36

tunnckoCore · 2018-03-02T10:41:25Z

fixes #35, closes #19

fixes terkelg#35, closes terkelg#19 Signed-off-by: Charlike Mike Reagent <[email protected]>

Signed-off-by: Charlike Mike Reagent <[email protected]>

FoRVaiS · 2018-03-02T20:58:49Z

7c042e3: this.clear() doesn't exist

$ /c/projects/_PLAYGROUND/PromptsPR#36/index.js
? Hello FöRVaiS » Hello!
√ Hello FöRVaiS ... Hello!
readline.js:1031
            throw err;
            ^

TypeError: this.clear is not a function
    at TextPrompt.close (C:\projects\_PLAYGROUND\PromptsPR#36\node_modules\prompts\lib\elements\prompt.js:45:17)
    at TextPrompt.submit (C:\projects\_PLAYGROUND\PromptsPR#36\node_modules\prompts\lib\elements\text.js:50:10)
    at Socket.keypress (C:\projects\_PLAYGROUND\PromptsPR#36\node_modules\prompts\lib\elements\prompt.js:30:16)
    at Socket.emit (events.js:160:13)
    at emitKeys (internal/readline.js:420:14)
    at emitKeys.next (<anonymous>)
    at Socket.onData (readline.js:1021:36)
    at Socket.emit (events.js:165:20)
    at addChunk (_stream_readable.js:269:12)
    at readableAddChunk (_stream_readable.js:256:11)

lukeed

Nice, but almost~!

Let's clean up that .gitignore... nothing there is needed.

And as mentioned, clear() doesn't exist. Looks like you forgot to import from util file.

FoRVaiS · 2018-03-02T21:55:17Z

Cause of Bug

In a TTY environment, such as VS Code's integrated terminal, Prompts works as intended.

In non-TTY environments like Git Bash:
If you (implicitly)? set the interpreter, #!/usr/bin/env node then run the file ./promptsTest.js Prompts starts to misbehave.

The cause of the bug comes from running a file with a Node shebang in a non-TTY environment.
Thus running a file with an absolute path /c/.../promptsTest.js or
with a bin command with this configuration:

"bin": {
    "prompts-test": "promptsTest.js"
}

then running the command

prompts-test

will both result in a broken Prompts application.

Workaround

Explicitly run a file with Node: node promptsTest.js.
I cannot find a workaround for bin commands.

tunnckoCore · 2018-03-03T01:47:16Z

nothing there is needed.

@lukeed what's not needed? 😆 *.log files may appear always. Coverage is up coming any way. Plus the NPM's lockfile.

Looks like you forgot to import from util file.

It was meant to be this.clear, but obviously we should try with clear from util.

I cannot find a workaround for bin commands.

@FoRVaiS, what about that?

"bin": {
  "prompts-test": "node promptsTest.js"
}

I have never tried it.

FoRVaiS · 2018-03-03T01:58:23Z

"bin": {
  "prompts-test": "node promptsTest.js"
}

left me an error saying that it couldn't find promptsTest.js. I fail to understand why that happens, since calling the file directly promptsTest.js would work perfectly (assuming we fix the non-TTY bug).

tunnckoCore · 2018-03-03T02:10:57Z

Try with absolute path or ./.

I had problems few times when just defining

"bin": {
  "foobar": "foobar.js"
}

So since then i always prepend ./ when doing CLIs.

FoRVaiS · 2018-03-03T02:26:22Z

"bin": {
  "prompts-test": "node ./promptsTest.js"
}

outputs this npm err. I don't get the chance to even run the test

$ npm publish; npm install -g local-promptstest
+ [email protected]
npm ERR! path C:\Program Files\nodejs\node_modules\local-promptstest\node .\index.js
npm ERR! code ENOENT
npm ERR! errno -4058
npm ERR! syscall chmod
npm ERR! enoent ENOENT: no such file or directory, chmod 'C:\Program Files\nodejs\node_modules\local-promptstest\node .\index.js'
npm ERR! enoent This is related to npm not being able to find a file.
npm ERR! enoent

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\Users\${user}\AppData\Roaming\npm-cache\_logs\2018-03-03T02_22_02_884Z-debug.log

terkelg

This part is pretty critical so I want to make sure that we get everything right and understand why/how everything works. No guessing at this level 👍

terkelg · 2018-03-03T03:47:05Z

.gitignore

@@ -1,3 +1,10 @@
 node_modules
 .DS_Store
 *.lock
+*.log


Please remove .gitignore from this commit. The ignored files are not related to this project. Include them in your own global gitignore if needed.

terkelg · 2018-03-03T03:54:45Z

lib/elements/prompt.js

@@ -16,7 +16,7 @@ class Prompt extends EventEmitter {
    this.out = process.stdout;

    const rl = readline.createInterface(this.in);
-    readline.emitKeypressEvents(this.in, this.rl);
+    readline.emitKeypressEvents(this.in, rl);


I still have to look into this. The second param to emitKeypressEvents is optional - do we even need it?

Also have to dig deeper into createInterface options like crlfDelay and terminal
https://nodejs.org/api/readline.html#readline_readline_createinterface_options

terkelg · 2018-03-03T06:28:46Z

How can I test/emulate a non-TTY environment on mac?

FoRVaiS · 2018-03-03T07:55:43Z

This looks like a rendering issue.

In this demo, pressing the down arrow twice lands me at the correct option but since it never rerenders, we see it stuck in the initial position. I added an output to report on the cursor's location but it never renders when prompt.select is up. They end up rendering after I end prompt.select.

Ignore the this.cursor + 1, forgot to remove it :P

FoRVaiS · 2018-03-03T08:58:53Z

Based on my 'bad rendering' theory, I've traced a potential problem to sisteransi.erase. The problem specifically being but potentially not limited to this.line.

Does anyone happen to know if using #!/usr/bin/env node at the top of a file is different in anyway compared to doing node promptsTest.js?

If there is, maybe these codes don't work when using a shebang?

const erase = {
  screen: `${ESC}2J`,
  up: `${ESC}1J`,
  down: `${ESC}J`,
  line: `${ESC}2K`,
  lineEnd: `${ESC}K`,
  lineStart: `${ESC}1K`,
  lines(count) {
    let clear = '';
    for (let i = 0; i < count; i++)
      clear += this.line + (i < count - 1 ? cursor.up() : '');
    if (count)
      clear += cursor.left;
    return clear;
  }
}

tunnckoCore · 2018-03-03T10:09:50Z

maybe these codes don't work when using a shebang?

@FoRVaiS i think that's the diff, yea. And colors.

Signed-off-by: Charlike Mike Reagent <[email protected]>

tunnckoCore · 2018-03-03T10:26:39Z

It's really frustrating to play on guesses... @FoRVaiS can you please try now ;d

Signed-off-by: Charlike Mike Reagent <[email protected]>

FoRVaiS · 2018-03-03T10:43:31Z

Email notifications are broken :x sorry

I can not even begin to explain what is going on here. I'll try more proposals tomorrow.

tunnckoCore · 2018-03-03T11:12:37Z

Thanks! Cool, starting to see fix :D

FoRVaiS · 2018-03-03T11:14:30Z

Nope, text and select are not okay yet.

prompts.select remains unscrollable (my theory is failure to re-render)
Both prompts.select and prompts.text fail to clear their previous lines (which is connected to my previous point, in theory)

I still haven't found solid evidence that the problems come from sisteransi.

tunnckoCore · 2018-03-03T11:20:36Z

lib/elements/prompt.js

@@ -39,8 +39,8 @@ class Prompt extends EventEmitter {
      if (this.in.isTTY) {
        this.in.setRawMode(false);
      } else {
-        // we can't use `utils.clear()`, because it expect `prompt` string
-        this.out.write(erase.line + cursor.prevLine());
+        this.clear = clear(this.prompt)


this.prompt should be available, because this.close is called after the this.render inside the submit()...

Also writing the result of clear to this.clear so it can be available in the next call of this.render

Signed-off-by: Charlike Mike Reagent <[email protected]>

tunnckoCore · 2018-03-03T11:23:20Z

All this should fix the text prompt i believe (why not select too ;d). But didn't looked on select yet.

FoRVaiS · 2018-03-03T11:27:23Z

4fdaf6e No change in output.

tunnckoCore · 2018-03-03T11:30:02Z

Arrrghh.

Does anyone know is AppVeyor using GitBash or such? Maybe adding AppVeyor would help the things a bit.

But in any way, such problems are hard cuz is user interaction..

FoRVaiS · 2018-03-03T23:53:56Z

So using both node sisteransi.js & ./sisteransi.js I was able to get a consistent result. Both types of execution for sure do not affect the way unicode behave. ( Probably why it's called UNI-code :P )

#!/usr/bin/env node

const { erase } = require('sisteransi');

const out = process.stdout;

console.log('Test 1');
console.log('Test 2');
console.log('Test 3');
console.log('Test 4');
console.log('Test 5');
console.log('Test 6');
out.write(erase.lines(3 + 1));

Expected Output:

Test 1
Test 2
Test 3

Output:

Test 1
Test 2
Test 3

FoRVaiS · 2018-03-04T00:27:50Z

I seriously don't get it... it seems as if render() is being queued or delayed?

TTY Environment/Terminal

Non-TTY Environment/Terminal

FoRVaiS · 2018-03-04T00:40:17Z

To demonstrate the 'queuing' behavior, I've modified:

prompts/lib/elements/text.js

Line 96 in 5b54227

this.out.write(this.clear + prompt);

to:
this.out.write('\n' + prompt);

Result:

@terkelg @olstenlarck does this bring us any closer to finding a solution?

FoRVaiS · 2018-03-04T01:38:59Z

@terkelg @olstenlarck So I'm 110% sure I've found the source of the problem. The problem is caused by stdin not reading keypress events in non-TTY instances until a user hits the enter key.

Using setRawMode skips the requirement of having to hit the enter key.

To be perfectly honest, this looks exponentially more like a Git Bash issue... Other popular prompt-type packages don't work as intended in Git Bash either. I have a feeling they might have abandoned a fix for Git Bash users :x.

Main thread to read:
nodejs/node#2996

This person gets the same queuing problem:
https://stackoverflow.com/questions/5006821/nodejs-how-to-read-keystrokes-from-stdin

terkelg · 2018-03-04T05:02:23Z

@FoRVaiS thank you for doing so much research on this one. Appreciate it! It's really hard for me to come up with a work-around for this when I can't reproduce the issue

FoRVaiS · 2018-03-04T06:49:14Z

@terkelg No worries, if a workaround/fix can be found, then it benefits us both. Otherwise, at least you would know what the exact problem is, when someone else has the same problem. :P

I'm unable to think of a workaround. The only solution that I can come up with, that doesn't require a redesign of the input system, would be to simulate an 'enter' keypress but all the packages that do keyboard simulation require Java's Robot package.

If a workaround is proposed, I'd be glad to try it out.

tunnckoCore · 2018-03-04T11:23:02Z

Great! 🎉 So much thank you for that!

In that case, I think we definitely should close the PR. It's not specific to this prompt module, but to Git Bash, so not make sense to touch on our side.

Adding notice on the README and pointing to this PR or to #35 issue.

fix: possible fix for isTTY and GitBash

065a86a

fixes terkelg#35, closes terkelg#19 Signed-off-by: Charlike Mike Reagent <[email protected]>

tunnckoCore mentioned this pull request Mar 2, 2018

Prompts doesn't work in global packages with Git Bash or Windows CMD #35

Closed

Charlike Mike Reagent added 2 commits March 2, 2018 13:36

fix: double lines with this.clear() on non TTY

7c042e3

Signed-off-by: Charlike Mike Reagent <[email protected]>

fix: update gitignores for coverage and basic things

2035c11

Signed-off-by: Charlike Mike Reagent <[email protected]>

lukeed requested changes Mar 2, 2018

View reviewed changes

terkelg reviewed Mar 3, 2018

View reviewed changes

remove gitignore updates

9cb058b

Signed-off-by: Charlike Mike Reagent <[email protected]>

try another way to clear previous line on non TTY

daf27a3

Signed-off-by: Charlike Mike Reagent <[email protected]>

tunnckoCore commented Mar 3, 2018

View reviewed changes

create reference and create this.clear inside the IF when non TTY

4fdaf6e

Signed-off-by: Charlike Mike Reagent <[email protected]>

terkelg closed this Mar 4, 2018

This was referenced Jul 9, 2018

Can't select change type with arrow keys commitizen/cz-cli#10

Closed

Looks like readline don't always triggers keypress events on Windows nodejs/node#2996

Closed

bitmvr mentioned this pull request Mar 20, 2019

Support mintty on MINGW64 SBoudrias/Inquirer.js#621

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: possible fix for isTTY and GitBash #36

fix: possible fix for isTTY and GitBash #36

tunnckoCore commented Mar 2, 2018

FoRVaiS commented Mar 2, 2018 •

edited

Loading

lukeed left a comment

FoRVaiS commented Mar 2, 2018 •

edited

Loading

tunnckoCore commented Mar 3, 2018 •

edited

Loading

FoRVaiS commented Mar 3, 2018

tunnckoCore commented Mar 3, 2018

FoRVaiS commented Mar 3, 2018

terkelg left a comment

terkelg Mar 3, 2018

terkelg Mar 3, 2018

terkelg commented Mar 3, 2018 •

edited

Loading

FoRVaiS commented Mar 3, 2018

FoRVaiS commented Mar 3, 2018 •

edited

Loading

tunnckoCore commented Mar 3, 2018 •

edited

Loading

tunnckoCore commented Mar 3, 2018 •

edited

Loading

FoRVaiS commented Mar 3, 2018 •

edited

Loading

tunnckoCore commented Mar 3, 2018

FoRVaiS commented Mar 3, 2018 •

edited

Loading

tunnckoCore Mar 3, 2018 •

edited

Loading

tunnckoCore commented Mar 3, 2018 •

edited

Loading

FoRVaiS commented Mar 3, 2018 •

edited

Loading

tunnckoCore commented Mar 3, 2018 •

edited

Loading

FoRVaiS commented Mar 3, 2018

FoRVaiS commented Mar 4, 2018

FoRVaiS commented Mar 4, 2018

FoRVaiS commented Mar 4, 2018 •

edited

Loading

terkelg commented Mar 4, 2018 •

edited

Loading

FoRVaiS commented Mar 4, 2018

tunnckoCore commented Mar 4, 2018

fix: possible fix for isTTY and GitBash #36

fix: possible fix for isTTY and GitBash #36

Conversation

tunnckoCore commented Mar 2, 2018

FoRVaiS commented Mar 2, 2018 • edited Loading

lukeed left a comment

Choose a reason for hiding this comment

FoRVaiS commented Mar 2, 2018 • edited Loading

Cause of Bug

Workaround

tunnckoCore commented Mar 3, 2018 • edited Loading

FoRVaiS commented Mar 3, 2018

tunnckoCore commented Mar 3, 2018

FoRVaiS commented Mar 3, 2018

terkelg left a comment

Choose a reason for hiding this comment

terkelg Mar 3, 2018

Choose a reason for hiding this comment

terkelg Mar 3, 2018

Choose a reason for hiding this comment

terkelg commented Mar 3, 2018 • edited Loading

FoRVaiS commented Mar 3, 2018

FoRVaiS commented Mar 3, 2018 • edited Loading

tunnckoCore commented Mar 3, 2018 • edited Loading

tunnckoCore commented Mar 3, 2018 • edited Loading

FoRVaiS commented Mar 3, 2018 • edited Loading

tunnckoCore commented Mar 3, 2018

FoRVaiS commented Mar 3, 2018 • edited Loading

tunnckoCore Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

tunnckoCore commented Mar 3, 2018 • edited Loading

FoRVaiS commented Mar 3, 2018 • edited Loading

tunnckoCore commented Mar 3, 2018 • edited Loading

FoRVaiS commented Mar 3, 2018

Expected Output:

Output:

FoRVaiS commented Mar 4, 2018

TTY Environment/Terminal

Non-TTY Environment/Terminal

FoRVaiS commented Mar 4, 2018

Result:

FoRVaiS commented Mar 4, 2018 • edited Loading

terkelg commented Mar 4, 2018 • edited Loading

FoRVaiS commented Mar 4, 2018

tunnckoCore commented Mar 4, 2018

FoRVaiS commented Mar 2, 2018 •

edited

Loading

FoRVaiS commented Mar 2, 2018 •

edited

Loading

tunnckoCore commented Mar 3, 2018 •

edited

Loading

terkelg commented Mar 3, 2018 •

edited

Loading

FoRVaiS commented Mar 3, 2018 •

edited

Loading

tunnckoCore commented Mar 3, 2018 •

edited

Loading

tunnckoCore commented Mar 3, 2018 •

edited

Loading

FoRVaiS commented Mar 3, 2018 •

edited

Loading

FoRVaiS commented Mar 3, 2018 •

edited

Loading

tunnckoCore Mar 3, 2018 •

edited

Loading

tunnckoCore commented Mar 3, 2018 •

edited

Loading

FoRVaiS commented Mar 3, 2018 •

edited

Loading

tunnckoCore commented Mar 3, 2018 •

edited

Loading

FoRVaiS commented Mar 4, 2018 •

edited

Loading

terkelg commented Mar 4, 2018 •

edited

Loading