-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
tty: Deno.setRaw(rid, mode) to turn on/off raw mode #3958
Conversation
7a553ae
to
8776116
Compare
cli/js/lib.deno.ns.d.ts
Outdated
/** An instance of `File` for stdout. */ | ||
export const stdout: File; | ||
/** An instance of `File` for stderr. */ | ||
export const stderr: File; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the minimality of isatty(rid)
. Maybe you can do something similar here, instead of defining this new class TTYInput
you could just add a new function... something like Deno.setRaw(rid)
. What do you think?
Additionally can you mark in the JSDOC any new APIs as
/** UNSTABLE: newly added API
*
* Check if a given resource is TTY
*/
export function isatty(rid: number): boolean;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think the original returning a restoration function after calling Deno.setRaw(rid)
works here? (In case I want to store the information for restoring without exposing? This would be a pure TS side change, and similar to what I did in the old PR, but without wrapping in a class)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think storing the restore state is so important. The user can do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think they can with current proposed API (unless we expose a full representation of <termios>
utilities) if we do not store for them? And there is the discrepancy between Unix and Windows here
Node does not provide the API for direct restoration either. I believe with tty.ReadStream::setRaw(false)
libuv simply sets back to the saved original mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
libuv simply sets back to the saved original mode
Yes we should do that too... I mean that the JS interface does not need to expose state restoration just boolean on/off is good enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The slight annoyance is that we will need to accompany StreamResource (including FsFile
) with such optional data. I'll see where would be the best place for me to place this piece of info.
b4429f4
to
4999c68
Compare
01128e5
to
dc4cddd
Compare
Added a basic test (unix only, I have no access to Windows). Added
|
9cc2e05
to
ec5e100
Compare
@kevinkassimo Looks good but I'm a little confused because there is still |
@ry I can probably update |
b4643e5
to
f1fbe6f
Compare
f1fbe6f
to
c6c9bb7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Is this PR considered ready? Have been resolving conflicts with master updates for quite a few times 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - sorry for the delay! Thanks!
function `Deno.setRaw(rid, mod)` is not implemented on Deno v1.35.1, rewrite insted of `Deno.stdin.setRaw(boolean b)` c.f. denoland/deno#3958
Difference from the first option:
Deno.setRaw(rid, mode)
to turn on/off raw modeDeno.isatty(rid)
that can test any resources, not just stdin/stdout/stderr (personally prefer deprecating currentDeno.isTTY()
)