feat(colors): add options for forcing color/no-color #990

thebanjomatic · 2022-04-04T00:05:37Z

This change checks for --color / --no-color as command-line arguments as a mechanism for forcing color in non-TTY situations, or forcing --no-color for TTY. Additionally, this change hooks into the FORCED_COLOR environment variable that is used by the supports-color npm package which is used by a variety of packages in the node ecosystem.

The intention of this PR is to alleviate some of the concerns from Issue #897 by allowing users of turborepo to enable color output on CI runners such as Github Actions which support color output (as mentioned in the fatih/color documentation). When github actions and local builds are both running in color, then the cached output should be consistently in color, compared to the current situation where its a mix of color and non-color output with a remote cache shared between CI and users building interactively.

The FORCED_COLOR environment variable is something that I find myself already setting when running turbo so that the streamed output is in color for the build and testing tools that I am running with it, so it seemed like a natural way to provide this argument without needing to modify the CLI parameters being passed to turbo.

In the event multiple methods are being used, the precidence is:

isTTY or not (provided by fatih/color)
$NO_COLOR (if set to anything, then color is disabled; also provided by fatih/color)
$FORCED_COLOR ("1", "2", "3", "true" will enable color, "0", or "false" will disable color; other values are ignored)
--color/--no-color (the last positional argument wins if both exist)

vercel · 2022-04-04T00:05:42Z

Someone is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

cli/internal/run/run.go

cli/internal/ui/ui.go

vercel · 2022-04-05T18:29:17Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/vercel/turbo-site/4eYh7sUmmf9eM8Y2VGkqJksmun4u
✅ Preview: https://turbo-site-git-fork-thebanjomatic-feat-colors.vercel.sh

thebanjomatic · 2022-04-05T22:46:57Z

.github/workflows/ci-go.yml

-        run: pnpm turbo run test --scope=cli
+        run: pnpm turbo -- run test --scope=cli --color


Only the positional arguments were being passed in before:

./turbow.sh "run" "test"

This now gets sent as:

./turbow.sh "run" "test" "--scope=cli" "--color"

Likely a change in behavior when switching to pnpm from yarn

thebanjomatic · 2022-04-05T22:48:37Z

The unit tests should all be fixed now, and I have also enabled the color output for the ci workflow in github actions for this repo:

gsoltis

I like where this is going!

cli/internal/ui/colors.go

cli/internal/ui/ui.go

cli/internal/ui/colors.go

cli/cmd/turbo/main.go

cli/internal/ui/ui.go

thebanjomatic · 2022-04-05T23:53:44Z

Thanks for your review @gsoltis I believe I have addressed all of your feedback. I mostly used your suggested naming, I think the only thing that was unspecified was the function to construct the cli.ColoredUi instance. For that, I went with Default() and BuildColoredUi(colorMode ColorMode) where Default() just returns BuildColorMode(ColorModeUndefined).

gsoltis

Awesome, just a couple minor things and I think this should be good to go

cli/cmd/turbo/main.go

gsoltis · 2022-04-06T03:49:18Z

cli/internal/ui/ui.go

+}
+
+func (into *stripAnsiWriter) Write(src []byte) (n int, err error) {
+	into.wrappedWriter.Write(ansiRegex.ReplaceAll(src, []byte{}))


This looks like it is losing the return values?

I think in general we prefer not to use named return variables for this reason and general readability.

This was somewhat intentional (ignoring the return value), but the named return variables was an oversight, I copied that out of the interface definition in io.go.

This is still a little bit weird because if there was no error writing, we actually need to return len(src), nil. Since the n returned from wrappedWriter.Write may be smaller than the number of bytes passed in, that is expected to be an error.

I changed things around so that if the wrapped call returns an error, we return that error and the associated number of bytes written instead of just ignoring the error completely, but the number of bytes written doesn't necessarily correlate to the source bytes in a way someone could meaningfully handle the error. My assumption is that Stdout and Stderr are very unlikely to produce an error when writing.

I added the reasoning above as a code comment as well.

I think that works.

This change checks for --color / --no-color as command-line arguments as a mechanism for forcing color in non-TTY situations, or forcing --no-color for TTY. Additionally, this change hooks into the FORCED_COLOR environment variable that is used by the [`supports-color` npm package](https://www.npmjs.com/package/supports-color) that is used by a variety of packages in the node ecosystem. The intention of this PR is to alleviate some of the concerns from Issue vercel#897 by allowing users of turborepo to enable color output on CI runners such as Github Actions which support color output (as mentioned in the [fatih/color documentation](https://github.com/fatih/color/tree/master#github-actions)). With github actions and local builds are both running in color, then the cached output should be consistently in color, compared to the current situation where its a mix of color and non-color output with a remote shared between CI and users building interactively. The FORCED_COLOR environment variable is something that I find myself already setting when running turbo so that the streamed output is in color for the build and testing tools that I am running with it, so it seemed like a natural way to provide this argument without needing to modify the CLI parameters being passed to turbo. In the event multiple methods are being used, the precedence is: * isTTY or not (provided by fatih/color) * $NO_COLOR (if set, then color is disabled) * $FORCED_COLOR ("1", "2", "3", "true" will enable color, "0", or "false" will disable color) * --color/--no-color (the last argument wins if both exist)

thebanjomatic · 2022-04-06T14:49:11Z

I also made an attempt at writing some documentation for the new flags

gsoltis

Awesome, thanks for submitting this!

vercel · 2022-04-06T16:14:17Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Updated	Deployment	Preview
turbo-site	✅ Deployed	Apr 6, 2022 at 4:15PM (UTC)	View on Vercel	View Preview

see PR vercel#990

thebanjomatic requested review from jaredpalmer, gsoltis and gaspar09 as code owners April 4, 2022 00:05

thebanjomatic force-pushed the feat/colors branch from 613d9c5 to eda1b78 Compare April 4, 2022 16:01

thebanjomatic commented Apr 4, 2022

View reviewed changes

cli/internal/run/run.go Show resolved Hide resolved

cli/internal/ui/ui.go Outdated Show resolved Hide resolved

cli/internal/ui/ui.go Outdated Show resolved Hide resolved

thebanjomatic force-pushed the feat/colors branch 2 times, most recently from 18160dd to e4b563d Compare April 5, 2022 22:40

thebanjomatic commented Apr 5, 2022

View reviewed changes

thebanjomatic force-pushed the feat/colors branch from e4b563d to 9bc2bac Compare April 5, 2022 22:56

gsoltis suggested changes Apr 5, 2022

View reviewed changes

thebanjomatic requested a review from gsoltis April 5, 2022 23:45

thebanjomatic force-pushed the feat/colors branch 4 times, most recently from cd811a1 to c16f6d1 Compare April 5, 2022 23:50

gsoltis suggested changes Apr 6, 2022

View reviewed changes

Adam J. Hines added 2 commits April 6, 2022 07:52

chore(ci): enable color in github actions for CI

93d10e5

thebanjomatic force-pushed the feat/colors branch from 2ef6389 to 93d10e5 Compare April 6, 2022 13:52

thebanjomatic requested a review from gsoltis April 6, 2022 14:00

docs(color): add docs for --color/--no-color flags

09a855f

thebanjomatic force-pushed the feat/colors branch from 733acda to 09a855f Compare April 6, 2022 14:47

gsoltis approved these changes Apr 6, 2022

View reviewed changes

vercel bot deployed to Preview April 6, 2022 16:14 View deployment

gsoltis merged commit 65b0e2f into vercel:main Apr 6, 2022

thebanjomatic deleted the feat/colors branch April 6, 2022 16:44

chelkyl added a commit to chelkyl/turborepo that referenced this pull request Apr 24, 2022

fix: sync with run.go

173ca8f

see PR vercel#990

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(colors): add options for forcing color/no-color #990

feat(colors): add options for forcing color/no-color #990

thebanjomatic commented Apr 4, 2022 •

edited

vercel bot commented Apr 4, 2022

vercel bot commented Apr 5, 2022 •

edited

thebanjomatic Apr 5, 2022

thebanjomatic commented Apr 5, 2022

gsoltis left a comment

thebanjomatic commented Apr 5, 2022

gsoltis left a comment

gsoltis Apr 6, 2022

thebanjomatic Apr 6, 2022

gsoltis Apr 6, 2022

thebanjomatic commented Apr 6, 2022

gsoltis left a comment

vercel bot commented Apr 6, 2022 •

edited

		run: pnpm turbo run test --scope=cli
		run: pnpm turbo -- run test --scope=cli --color

feat(colors): add options for forcing color/no-color #990

feat(colors): add options for forcing color/no-color #990

Conversation

thebanjomatic commented Apr 4, 2022 • edited

vercel bot commented Apr 4, 2022

vercel bot commented Apr 5, 2022 • edited

thebanjomatic Apr 5, 2022

Choose a reason for hiding this comment

thebanjomatic commented Apr 5, 2022

gsoltis left a comment

Choose a reason for hiding this comment

thebanjomatic commented Apr 5, 2022

gsoltis left a comment

Choose a reason for hiding this comment

gsoltis Apr 6, 2022

Choose a reason for hiding this comment

thebanjomatic Apr 6, 2022

Choose a reason for hiding this comment

gsoltis Apr 6, 2022

Choose a reason for hiding this comment

thebanjomatic commented Apr 6, 2022

gsoltis left a comment

Choose a reason for hiding this comment

vercel bot commented Apr 6, 2022 • edited

thebanjomatic commented Apr 4, 2022 •

edited

vercel bot commented Apr 5, 2022 •

edited

vercel bot commented Apr 6, 2022 •

edited