-
Notifications
You must be signed in to change notification settings - Fork 13.2k
Support FORCE_COLOR #63055
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
Support FORCE_COLOR #63055
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds support for the FORCE_COLOR environment variable to control tsc’s “pretty” (colored) output, while ensuring NO_COLOR continues to disable color even when FORCE_COLOR is present.
Changes:
- Update pretty-output defaulting logic to honor
FORCE_COLOR(and keepNO_COLORas an override). - Add command-line unit tests for
FORCE_COLORandNO_COLOR+FORCE_COLORprecedence. - Add new reference baselines for the new test scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/compiler/executeCommandLine.ts | Updates defaultIsPretty to consider NO_COLOR and FORCE_COLOR. |
| src/testRunner/unittests/tsc/commandLine.ts | Adds verifyTsc cases covering FORCE_COLOR and precedence with NO_COLOR. |
| tests/baselines/reference/tsc/commandLine/adds-color-when-FORCE_COLOR-is-set.js | Baseline output for the new FORCE_COLOR scenario. |
| tests/baselines/reference/tsc/commandLine/does-not-add-color-when-NO_COLOR-is-set-even-if-FORCE_COLOR-is-set.js | Baseline output ensuring NO_COLOR disables color even when FORCE_COLOR is set. |
| verifyTsc({ | ||
| scenario: "commandLine", | ||
| subScenario: "adds color when FORCE_COLOR is set", | ||
| sys: () => | ||
| TestServerHost.createWatchedSystem(emptyArray, { | ||
| environmentVariables: new Map([["FORCE_COLOR", "true"]]), | ||
| }), | ||
| commandLineArgs: emptyArray, | ||
| }); |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new "adds color when FORCE_COLOR is set" test uses TestServerHost.createWatchedSystem, whose writeOutputIsTTY() always returns true. That means the output would already be colored even without FORCE_COLOR, so this test doesn't actually validate the new FORCE_COLOR behavior (forcing color in a non-TTY scenario). Consider overriding writeOutputIsTTY to return false for this test (or enhancing the host to support a non-TTY mode) so the baseline would differ without FORCE_COLOR and the test would catch regressions.
| if (sys.getEnvironmentVariable("NO_COLOR")) { | ||
| return false; | ||
| } | ||
| if (sys.getEnvironmentVariable("FORCE_COLOR")) { |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FORCE_COLOR is treated as enabled for any non-empty value, but in common FORCE_COLOR semantics a value of "0" explicitly disables color. With the current check, FORCE_COLOR=0 will incorrectly force pretty output on. Consider parsing the value and treating "0" (and possibly "false") as disabling, while still allowing "1"/"2"/"3" to force-enable.
| if (sys.getEnvironmentVariable("FORCE_COLOR")) { | |
| const forceColor = sys.getEnvironmentVariable("FORCE_COLOR"); | |
| if (forceColor !== undefined) { | |
| const normalized = forceColor.trim().toLowerCase(); | |
| if (normalized === "0" || normalized === "false") { | |
| return false; | |
| } |
Same as microsoft/typescript-go#2596