lib: support `FORCE_COLOR` for non TTY streams by MoLow · Pull Request #48034 · nodejs/node · GitHub
Skip to content

lib: support FORCE_COLOR for non TTY streams#48034

Merged
nodejs-github-bot merged 5 commits into
nodejs:mainfrom
MoLow:force-colors
May 18, 2023
Merged

lib: support FORCE_COLOR for non TTY streams#48034
nodejs-github-bot merged 5 commits into
nodejs:mainfrom
MoLow:force-colors

Conversation

@MoLow

@MoLow MoLow commented May 16, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

The current behavior of FORCE_COLOR is to take effect when stdout/stderr are TTY's

according to the documentation:

The FORCE_COLOR environment variable is used to enable ANSI colorized output.

this PR moves the check for the value of FORCE_COLOR before testing isTTY - and extracts the logic to a central utility.

Refs: #31409 (comment)

@nodejs-github-bot

nodejs-github-bot commented May 16, 2023

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels May 16, 2023
MoLow
MoLow commented May 16, 2023
View reviewed changes
Comment thread test/parallel/test-repl-envvars.js Outdated
}

tests.forEach(run);
tests.forEach((testCase) => test(inspect(testCase.env), () => run(testCase)));

@MoLow MoLow May 16, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

tests did not play well when running in parallel. used node:test to run them sequentially

@MoLow

MoLow commented May 16, 2023

Copy link
Copy Markdown
Member Author

/CC @nodejs/util @nodejs/console

anonrig
anonrig reviewed May 16, 2023
View reviewed changes
Comment thread lib/internal/util/colors.js Outdated
Comment thread lib/internal/util/colors.js Outdated
Comment thread lib/internal/util/colors.js
Comment thread lib/internal/util/colors.js Outdated
ljharb
ljharb approved these changes May 16, 2023
View reviewed changes
Comment thread lib/repl.js Outdated
@MoLow MoLow requested a review from anonrig May 17, 2023 05:31
@MoLow MoLow added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels May 17, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 17, 2023
@nodejs-github-bot

nodejs-github-bot commented May 17, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/51852/

aduh95
aduh95 reviewed May 17, 2023
View reviewed changes
Comment thread test/fixtures/errors/force_colors.js Outdated
@MoLow @aduh95
Update test/fixtures/errors/force_colors.js
4c6d273 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@MoLow MoLow added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label May 17, 2023
@MoLow MoLow added the request-ci Add this label to start a Jenkins CI on a PR. label May 17, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 17, 2023
@nodejs-github-bot

nodejs-github-bot commented May 17, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/51856/

This was referenced May 23, 2023
Open
Open
targos pushed a commit that referenced this pull request May 30, 2023
@MoLow @targos
lib: support FORCE_COLOR for non TTY streams
a90010a 
PR-URL: #48034
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@targos targos mentioned this pull request Jun 4, 2023
Merged
@danielleadams

danielleadams commented Jul 5, 2023

Copy link
Copy Markdown
Contributor

This broke tests when pulling into v18, so it needs a backport.

@MoLow

MoLow commented Jul 6, 2023

Copy link
Copy Markdown
Member Author

@danielleadams It seems v18.x-staging is missing some other commits this PR depends on (b6738c1, 03db049) is that delibirate?
anyway I will create a backport PR for all the three

@MoLow

MoLow commented Jul 6, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

@danielleadams cherry-picking this sequence of commits lands clearly with no conflicts and all tests pass, so I assume the staging branch is missing many many more parts

334bb17
d51541b
d1eaded
2e8f8eb
6d06586
102540a
af8ed02
7ec93fb
57508fb
b6738c1
46a3cff
d3b0a2a
9e5e2f1
03db049
05693ac

@danielleadams

danielleadams commented Jul 6, 2023

Copy link
Copy Markdown
Contributor

Talked to @MoLow offline, but staging branch is ready for backport now.

MoLow added a commit to MoLow/node that referenced this pull request Jul 6, 2023
@MoLow
lib: support FORCE_COLOR for non TTY streams
d7d3c0a 
PR-URL: nodejs#48034
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@MoLow MoLow mentioned this pull request Jul 6, 2023
Closed
MoLow added a commit to MoLow/node that referenced this pull request Jul 9, 2023
@MoLow
lib: support FORCE_COLOR for non TTY streams
277e5be 
PR-URL: nodejs#48034
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
MoLow added a commit to MoLow/node that referenced this pull request Jul 9, 2023
@MoLow
lib: support FORCE_COLOR for non TTY streams
69f5fad 
PR-URL: nodejs#48034
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@MoLow MoLow mentioned this pull request Jul 9, 2023
Closed
MoLow added a commit to MoLow/node that referenced this pull request Jul 9, 2023
@MoLow
lib: support FORCE_COLOR for non TTY streams
b23dcb9 
PR-URL: nodejs#48034
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams pushed a commit that referenced this pull request Jul 12, 2023
@MoLow @danielleadams
lib: support FORCE_COLOR for non TTY streams
c46b31f 
PR-URL: #48034
Backport-PR-URL: #48684
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@danielleadams danielleadams mentioned this pull request Jul 12, 2023
Merged
@aweebit aweebit mentioned this pull request Aug 7, 2023
Closed
@trentm trentm mentioned this pull request Nov 3, 2023
Merged
@addaleax

addaleax commented Jan 22, 2024

Copy link
Copy Markdown
Member

@MoLow Small heads up, this seems to have broken NO_COLOR support for the REPL (i.e. NO_COLOR=1 node now uses color mode when previously it didn't)

@MoLow

MoLow commented Jan 23, 2024

Copy link
Copy Markdown
Member Author

@addaleax I am not able to reproduce this:
image
what specifically are you running inside the repl?

@addaleax

addaleax commented Jan 23, 2024

Copy link
Copy Markdown
Member

@MoLow MoLow mentioned this pull request Jan 26, 2024
Merged
@dgozman dgozman mentioned this pull request Mar 25, 2024
Merged
dgozman added a commit to microsoft/playwright that referenced this pull request Mar 25, 2024
@dgozman
feat(test runner): do not mock tty in the worker process (#30107)
911d8ef 
This was historically done to make `console.log()` have colors. However,
this makes any other code that checks `process.stdout.isTTY` incorrectly
assume real TTY support.

Node18 and Node20 now respect `FORCE_COLOR=1` in console, so our default
behavior of forcing colors in the worker process just works out of the
box. See nodejs/node#48034.
@richardlau richardlau mentioned this pull request Jan 7, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aduh95 aduh95 aduh95 left review comments
@ljharb ljharb ljharb approved these changes
@benjamingr benjamingr benjamingr approved these changes
@anonrig anonrig anonrig approved these changes
@cjihrig cjihrig Awaiting requested review from cjihrig

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@MoLow @nodejs-github-bot @danielleadams @addaleax @ljharb @benjamingr @anonrig @aduh95