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

why not use the standard tty package in combination with opening /dev/tty #15

Closed
sam-github opened this issue Jan 24, 2020 · 7 comments
Closed

Comments

@sam-github
Copy link

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.

core/node (master $ u=) % node -p 'require("tty").WriteStream(require("fs").openSync("/dev/tty", "w")).columns'     
174
core/node (master $ u=) % node -p 'process.exit(require("tty").WriteStream(require("fs").openSync("/dev/tty", "w")).columns)' < /dev/null >/dev/null 2>/dev/null; echo $?
174
core/node (master $ u=) % nvm run 13 -p 'process.exit(require("tty").WriteStream(require("fs").openSync("/dev/tty", "w")).columns)' < /dev/null >/dev/null 2>/dev/null; echo $?
174
core/node (master $ u=) % nvm run 12 -p 'process.exit(require("tty").WriteStream(require("fs").openSync("/dev/tty", "w")).columns)' < /dev/null >/dev/null 2>/dev/null; echo $?
174
core/node (master $ u=) % nvm run 10 -p 'process.exit(require("tty").WriteStream(require("fs").openSync("/dev/tty", "w")).columns)' < /dev/null >/dev/null 2>/dev/null; echo $?
174
core/node (master $ u=) % nvm run 8 -p 'process.exit(require("tty").WriteStream(require("fs").openSync("/dev/tty", "w")).columns)' < /dev/null >/dev/null 2>/dev/null; echo $?

174
core/node (master $ u=) % nvm run 6 -p 'process.exit(require("tty").WriteStream(require("fs").openSync("/dev/tty", "w")).columns)' < /dev/null >/dev/null 2>/dev/null; echo $?

174
@sam-github
Copy link
Author

Got here from nodejs/node#31459 (comment)

@sindresorhus
Copy link
Owner

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 w? We don't need to write to it. It should probably be:

node -p 'process.exit(require("tty").WriteStream(require("fs").openSync("/dev/tty", fs.constants.O_EVTONLY | fs.constants.O_NONBLOCK)).columns)' < /dev/null >/dev/null 2>/dev/null; echo $?

To match https://github.com/sindresorhus/macos-term-size/blob/19a58cdeff095aa6ca11baec53da010d9762be9c/term-size.c#L9

@Qix-
Copy link

Qix- commented Feb 2, 2020

The ioctl versions are going to be more robust as they interact directly with the underlying character device. /dev/tty is usually a character device wrapper, and generally points to the same device - though I'd be wary of edge cases where /dev/tty and stdout/stderr do not refer to the same TTY device. Such edge cases might include convoluted container setups where /dev is not mapped properly/at all - in which case, ioctl calls are still going to work and thus be more robust.

However, interacting with /dev/tty is going to be leagues faster than spawning a process. The "perfect" approach would be to wrap the ioctl calls in a native module instead of spawning a process, but I understand the headache that comes with that.

I'd keep/improve the ioctl method, personally. Kind of cool you can do this directly with Node, though.

@sam-github
Copy link
Author

@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.

The ioctl versions are going to be more robust as they interact directly with the underlying character device.

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! :-)

/dev/tty is usually a character device wrapper, and generally points to the same device - though I'd be wary of edge cases where /dev/tty and stdout/stderr do not refer to the same TTY device.

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.

@Qix-
Copy link

Qix- commented Feb 2, 2020

It's not about the stream functionality or the ioctl bindings. The problem I foresee is the mapping of the /dev/tty path itself. Sorry if I wasn't clear ^^

@sam-github
Copy link
Author

sam-github commented Feb 3, 2020

The problem I foresee is the mapping of the /dev/tty path itself.

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?

@Qix-
Copy link

Qix- commented Feb 3, 2020

You're right, I mis-read the existing C utilities. That just means the existing implementation is also susceptible to cases where /dev/tty is not present (as I said, this might happen if /dev is not properly mounted or is a non-standard devfs implementation).

This can be somewhat alleviated by using the ctermid(NULL) call (provided in stdlib.h) in lieu of the hardcoded "/dev/tty" (in most cases, it will return the same string). This is the one improvement I'd make - however, I do not know of a Node.js equivalent to that function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants