-
-
Notifications
You must be signed in to change notification settings - Fork 15
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
why not use the standard tty package in combination with opening /dev/tty #15
Comments
Got here from nodejs/node#31459 (comment) |
Nice! I did not know about that. I does seem to work, but I wonder if it has any downsides compared to the current approach. Why are you specifying
|
The However, interacting with I'd keep/improve the |
@sindresorhus I just did POC code, your version looks better to me. And I agree, having to create a stream to get the window size is a bit obscure, a direct binding in tty or fs to call against an fd would be nice, but still, the basic feature is there across all LTS versions.
Using the built-in Node.js code just calls through javascript into uv, which does https://github.com/nodejs/node/blob/99c8c6d80ff97ac7d53e01722142ac37756aabf1/deps/uv/src/unix/tty.c#L269-L274, which is identical to what term-size.c does, https://github.com/sindresorhus/macos-term-size/blob/19a58cdeff095aa6ca11baec53da010d9762be9c/term-size.c#L9-L16 If there are down sides, they aren't obvious. The upsides are pretty obvious, though! :-)
This doesn't change the edge cases, its basically a more direct way of calling the same code. I assume term-size isn't spawned unless stdout doesn't have a .columns property. |
It's not about the stream functionality or the ioctl bindings. The problem I foresee is the mapping of the |
That is specifically what I don't understand. Existing code spawns a child process which calls ioctl on /dev/tty. (EDIT: "proposed") Replacement code uses js bindings to call (the same) ioctl on /dev/tty. What new problem do you foresee? |
You're right, I mis-read the existing C utilities. That just means the existing implementation is also susceptible to cases where This can be somewhat alleviated by using the |
I can't print the output to stdout, because its redirected :-), so I use the exit status.
Tested on Linux with
node >= 6.x
. Also tried rows. I'd expect this to be ancient unix behaviour shared with OS X.The text was updated successfully, but these errors were encountered: