use readline for prompt() via magic #96
Conversation
There was a problem hiding this comment.
LGTM, I see Toby and Alex in the blame for adjacent files, may want to see if either of them have opinions. Did you confirm that this resolves the bug in question (just to be sure we isolated the right cause)?
I can't decide if the test failure is an actual problem or just needs the relevant prompt tests to be updated. It seems like maybe the new prompt style does not support ctrl+c to exit, changing that behavior would be bad to my eye, but I trust your judgement more than mine here. 🤷
| @@ -1,34 +1,19 @@ | |||
| const { readSync, writeSync, openSync } = require("fs"); | |||
| const { isatty } = require("tty"); | |||
| const { readSync, writeSync, openSync } = require('fs'); | |||
| if (newIndex >= 0 && newIndex <= str.length) { | ||
| index = newIndex; | ||
| } | ||
| while (result == null) rd.readNext(); |
There was a problem hiding this comment.
Is this eqeq intentional? I don't see any case that the coercion would be desirable.
If it is intentional, maybe leave a comment explaining it.
There was a problem hiding this comment.
It is intentional. It's what's called a "nullish" check. it's somewhat unstandard but handy to know.
a == null returns true if a is nullish (null or undefined).
The old logic for
promptwas pretty bad andreadlinehas all of the logic we need + more so I figured we should just use it instead of our own logic.This does come with a contingency though - this works because
stream.Readable,events.EventEmitter, andreadline.Interfacesynchronously handle data pushed to streams. If that were to change (e.g. debouncing event dispatches) then this would fail.