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

tty: stdio properties defined inside exec() child #15636

Closed
wants to merge 4 commits into from
Closed

tty: stdio properties defined inside exec() child #15636

wants to merge 4 commits into from

Conversation

dusansimic
Copy link

Properties inside exec() child were not predefined because
process.stdin is a PIPE.

This commit suggests predefining them inside getStdin() function.

Fixes: #2333

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

Properties inside exec() child were not predefined because
process.stdin is a PIPE.

This commit suggests predefining them inside getStdin() function.

Fixes: #2333
@nodejs-github-bot nodejs-github-bot added the process Issues and PRs related to the process subsystem. label Sep 27, 2017
@jasnell jasnell requested a review from refack September 28, 2017 12:40
@jasnell jasnell added the tty Issues and PRs related to the tty subsystem. label Sep 28, 2017
@jasnell
Copy link
Member

jasnell commented Sep 28, 2017

Ping @nodejs/tsc ... thoughts on this?

@cjihrig
Copy link
Contributor

cjihrig commented Sep 28, 2017

It seems incorrect to me to unconditionally set these like this.

@Fishrock123
Copy link
Member

Fishrock123 commented Sep 28, 2017

@dusansimic These are only available to tty.ReadStreams on purpose, see https://nodejs.org/dist/latest-v8.x/docs/api/tty.html#tty_class_tty_readstream

If we are going to add them more universally, they should be added to all net.Sockets, I think.

@refack
Copy link
Contributor

refack commented Sep 28, 2017

Hello @dusansimic and welcome. Thanks for submitting this PR 🥇
If you haven't already, you might find it informative to read the CONTRIBUTING guide (especially the part about "discuss and update").

P.S. If you have any question feel free to contact me directly (@-mention me, or ping me on IRC)

@Fishrock123
Copy link
Member

Fwiw I personally would prefer to just have these be set only on tty streams. I don't think they make much sense elsewhere.

Properties inside exec() child were not predefined because
process.stdin is a PIPE.

This commit suggests predefining them inside getStdin() function.

Fixes: #2333
Properties inside exec() child were not predefined because
process.stdin is a PIPE.

This commit suggests predefining them inside getStdin() function.

Fixes: #2333
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting .isTTY etc. on all net.Socket objects is not right.

@Fishrock123
Copy link
Member

I'm going to close this, I don't think this is the approach we are collectively going to take. Simple detection already works fine for these properties.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
process Issues and PRs related to the process subsystem. tty Issues and PRs related to the tty subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tty: stdio properties are undefined inside exec() child
7 participants