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

Allow testing of individual streams #64

Merged
merged 1 commit into from
Oct 18, 2017
Merged

Allow testing of individual streams #64

merged 1 commit into from
Oct 18, 2017

Conversation

Qix-
Copy link
Member

@Qix- Qix- commented Aug 12, 2017

Closes #51. This is a breaking change.


This PR adds the ability to test individual output streams for validity. This is one particular approach - see the end of this PR for another non-breaking proposed approach.

This exposes the new API as:

const supportsColor = {
    supportsColor: (stream) => {...},
    stdout: supportsColor(process.stdout),
    stderr: supportsColor(process.stderr)
}

So the only change necessary to migrate would be to change

const supportsColor = require('supports-color');

if (supportsColor) {
    // ...
}

if (supportsColor.level >= 2) {
    // ...
}

to either

const supportsColor = require('supports-color');

if (supportsColor.stdout) {
    // ...
}

if (supportsColor.stdout.level >= 2) {
    // ...
}

or simply

const supportsColor = require('supports-color').stdout;

So as I mentioned earlier, this is one of two possible ways we can approach this problem. In my opinion, this PR is the correct way to approach the problem, albeit a breaking change.

However, the alternative would be to introduce a second script that can be included directly:

const stderrSupportsColor = require('supports-color/stream')(process.stderr);

Though philosophically that is a bandaid fix, I foresee this package going away sometime within the next few years due to progress in the TTY realm, so I feel much less strongly about the approach itself.

If this is the route we want to take as opposed to this PR, that's totally fine - just let me know.

@Qix- Qix- requested a review from sindresorhus August 12, 2017 23:16
@sindresorhus
Copy link
Member

Remind me again, why do we need this? In the whole time Chalk has existed we've gotten only one request for this.

@Qix-
Copy link
Member Author

Qix- commented Aug 13, 2017

@sindresorhus we want it for debug. Functionality changes depending on if stderr is a TTY. If it's not, we use supports-color to detect 256 color support for more colors per namespace. The module is intended to be used with different forms of fd redirection.

Plus, it's the correct thing to do IMO.

@sindresorhus sindresorhus merged commit 93fc593 into master Oct 18, 2017
@sindresorhus sindresorhus deleted the streams branch October 18, 2017 05:56
@sindresorhus
Copy link
Member

Thanks for your patience while we were calculating Llama Expectoration Trajectory.

sindresorhus pushed a commit that referenced this pull request Oct 18, 2017
sindresorhus added a commit that referenced this pull request Oct 18, 2017
targos added a commit to targos/citgm that referenced this pull request Nov 19, 2017
The library now returns an object with "stdout" and "stderr" properties
Refs: chalk/supports-color#64
MylesBorins pushed a commit to nodejs/citgm that referenced this pull request Nov 19, 2017
The library now returns an object with "stdout" and "stderr" properties
Refs: chalk/supports-color#64
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Take stream as argument so we can check for process.stderr
2 participants