Skip to content

Conversation

@jamestalmage
Copy link
Contributor

This passes terminal width information to, and enables ansi-color
support in, the forked process. It makes the output of time-require
much prettier when used in a fork.

This passes terminal width information to, and enables ansi-color
support in, the forked process. It makes the output of `time-require`
much prettier when used in a fork.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure why we forward both child output streams to stdout. I do it here simply because it is what we were doing before. Making that change will impact a lot of tests, and is a separate PR.

In my mind what we are currently doing makes the least sense of the the available options. I think we should either:

  1. pipe child.stdout -> process.stdout, and child.stderr -> process.stderr.
  2. pipe both to process.stderr. This keeps process.stdout pristine and will not interfere with our TAP output.

Copy link
Member

Choose a reason for hiding this comment

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

Can you open an issue with the contents of this comment? I'm not sure to be honest.

@jamestalmage
Copy link
Contributor Author

@sindresorhus - this seems like a no brainer to me. What we were doing before was essentially piping to stdout. This just uses the child_process api to do the same thing with less code.

I'm gonna merge unless you object.

sindresorhus added a commit that referenced this pull request Nov 27, 2015
Pipe forked IO streams instead of forwarding `data` events.
@sindresorhus sindresorhus merged commit 306f28d into avajs:master Nov 27, 2015
@jamestalmage
Copy link
Contributor Author

Or that! 😄

@jamestalmage jamestalmage deleted the pipe-forked-io-streams branch November 27, 2015 06:40
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

Successfully merging this pull request may close these issues.

2 participants