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.

Sign up for GitHub

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

Jump to bottom

lib: support FORCE_COLOR for non TTY streams #48034

Merged
merged 5 commits into from
May 18, 2023
Merged

lib: support FORCE_COLOR for non TTY streams #48034

merged 5 commits into from
May 18, 2023

Conversation

MoLow
Copy link
Member

@MoLow MoLow commented May 16, 2023 •
edited
Loading

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
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@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
test/parallel/test-repl-envvars.js Outdated
@@ -72,4 +81,4 @@ function run(test) {
});
}

tests.forEach(run);
tests.forEach((testCase) => test(inspect(testCase.env), () => run(testCase)));
Copy link
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
Copy link
Member Author

MoLow commented May 16, 2023

/CC @nodejs/util @nodejs/console

anonrig
anonrig reviewed May 16, 2023
View reviewed changes
lib/internal/util/colors.js Outdated Show resolved Hide resolved
lib/internal/util/colors.js Outdated Show resolved Hide resolved
lib/internal/util/colors.js Show resolved Hide resolved
lib/internal/util/colors.js Outdated Show resolved Hide resolved
ljharb
ljharb approved these changes May 16, 2023
View reviewed changes
lib/repl.js Outdated Show resolved Hide resolved
@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
Copy link
Collaborator

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

@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
Copy link
Collaborator

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

fasenderos pushed a commit to fasenderos/node that referenced this pull request May 22, 2023
@MoLow @fasenderos
lib: support FORCE_COLOR for non TTY streams
dd6d4d0 
PR-URL: nodejs#48034
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
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 added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Jul 5, 2023
@danielleadams
Copy link
Contributor

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

@MoLow
Copy link
Member Author

MoLow commented Jul 6, 2023

@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
Copy link
Member Author

MoLow commented Jul 6, 2023 •
edited
Loading

@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
Copy link
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 MoLow added backport-open-v18.x Indicate that the PR has an open backport. and removed backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. labels Jul 6, 2023
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 added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed backport-open-v18.x Indicate that the PR has an open backport. labels Jul 12, 2023
@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
Copy link
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
Copy link
Member Author

MoLow commented Jan 23, 2024

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

@addaleax
Copy link
Member

@MoLow Yeah, console.log() works perfectly (its doesn't inherit colorization from the REPL), but if you enter e.g. just 111 as the input instead of console.log(111) you'll see that the output is colorized in both cases with 20.10.0, as opposed to e.g. 20.2.0

@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.
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

@anonrig anonrig anonrig approved these changes

@benjamingr benjamingr benjamingr approved these changes

@ljharb ljharb ljharb 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. backported-to-v18.x PRs backported to the v18.x-staging branch. 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