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

util: detecting terminal capabilities in styleText #51959

Closed
wants to merge 1 commit into from
Closed

util: detecting terminal capabilities in styleText #51959

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion doc/api/util.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1803,7 +1803,10 @@ added: REPLACEME
* `format` {string} A text format defined in `util.inspect.colors`.
* `text` {string} The text to to be formatted.

This function returns a formatted text considering the `format` passed.
This function returns a formatted text considering the `format` passed
for printing in a terminal, it is aware of the terminal's capabilities
and act according to the configuration set via `NO_COLORS`,
`NODE_DISABLE_COLORS` and `FORCE_COLOR` environment variables.

```mjs
import { styleText } from 'node:util';
Expand Down
6 changes: 6 additions & 0 deletions lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,12 @@ function pad(n) {
* @returns {string}
*/
function styleText(format, text) {
Copy link
Member

Choose a reason for hiding this comment

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

if we add this we probably want to pass a force parameter (not an env var)

Copy link
Member Author

Choose a reason for hiding this comment

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

ditto

const env = process.env;
if (!process.stdout.isTTY ||
Copy link
Member

Choose a reason for hiding this comment

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

please use shouldColorize from internal/util/colors, it takes all these into account but also the actual color depth of the tty.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will follow this suggestion based on the feedback for my proposal below

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure it's the right thing to systematically look at process.stdout. styleText, as an utility function, should not (always) depend on that.

Copy link
Member

@RafaelGSS RafaelGSS Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do you mean the user should perform this check by themselves?

Copy link
Member

Choose a reason for hiding this comment

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

When using such a utility, you aren't necessarily writing on to stdout. You might be writing to a file, and you might want to use coloring even without a tty

Copy link
Member

Choose a reason for hiding this comment

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

That's a fair point.

Copy link
Member Author

Choose a reason for hiding this comment

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

What about adding a boolean parameter to the function set to true by default to check the TTY: styleText(format, text, validateTTY)? So we offer the option to use the function with or without the proposed validation?

The idea behind this validation is to provide the mechanism by default to Node users, saving them time with some code that would be a must in the most common use case of this utility function.

I will be waiting for your feedback, @targos and @MoLow to implement such suggestion

Copy link
Member

Choose a reason for hiding this comment

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

SGTM

(!env.FORCE_COLOR && (env.NO_COLOR || env.NODE_DISABLE_COLORS))) {
return text;
}

validateString(text, 'text');
const formatCodes = inspect.colors[format];
if (formatCodes == null) {
Expand Down
30 changes: 29 additions & 1 deletion test/parallel/test-util-styletext.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@
require('../common');
const assert = require('assert');
const util = require('util');
const { execSync } = require('child_process');
const { EOL } = require('os');
const styled = '\u001b[31mtest\u001b[39m';
const noChange = 'test';


[
undefined,
Expand All @@ -13,7 +18,7 @@
() => {},
{},
[],
].forEach((invalidOption) => {

Check failure on line 21 in test/parallel/test-util-styletext.js

View workflow job for this annotation

GitHub Actions / test-macOS 

--- stderr ---
node:assert:126
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Missing expected exception.
    at /Users/runner/work/node/node/test/parallel/test-util-styletext.js:22:10
    at Array.forEach (<anonymous>)
    at Object.<anonymous> (/Users/runner/work/node/node/test/parallel/test-util-styletext.js:21:3)
    at Module._compile (node:internal/modules/cjs/loader:1368:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1426:10)
    at Module.load (node:internal/modules/cjs/loader:1205:32)
    at Module._load (node:internal/modules/cjs/loader:1021:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:142:12)
    at node:internal/main/run_main_module:28:49 {
  generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: undefined,
  expected: { code: 'ERR_INVALID_ARG_VALUE' },
  operator: 'throws'
}

Node.js v22.0.0-pre
Command: out/Release/node --test-reporter=spec --test-reporter-destination=stdout --test-reporter=./tools/github_reporter/index.js --test-reporter-destination=stdout /Users/runner/work/node/node/test/parallel/test-util-styletext.js

Check failure on line 21 in test/parallel/test-util-styletext.js

View workflow job for this annotation

GitHub Actions / test-linux 

--- stderr ---
node:assert:126
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Missing expected exception.
    at /home/runner/work/node/node/test/parallel/test-util-styletext.js:22:10
    at Array.forEach (<anonymous>)
    at Object.<anonymous> (/home/runner/work/node/node/test/parallel/test-util-styletext.js:21:3)
    at Module._compile (node:internal/modules/cjs/loader:1368:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1426:10)
    at Module.load (node:internal/modules/cjs/loader:1205:32)
    at Module._load (node:internal/modules/cjs/loader:1021:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:142:12)
    at node:internal/main/run_main_module:28:49 {
  generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: undefined,
  expected: { code: 'ERR_INVALID_ARG_VALUE' },
  operator: 'throws'
}

Node.js v22.0.0-pre
Command: out/Release/node --test-reporter=spec --test-reporter-destination=stdout --test-reporter=./tools/github_reporter/index.js --test-reporter-destination=stdout /home/runner/work/node/node/test/parallel/test-util-styletext.js

Check failure on line 21 in test/parallel/test-util-styletext.js

View workflow job for this annotation

GitHub Actions / test-asan 

--- stderr ---
node:assert:126
  throw new AssertionError(obj);
  ^

AssertionError [ERR_ASSERTION]: Missing expected exception.
    at /home/runner/work/node/node/test/parallel/test-util-styletext.js:22:10
    at Array.forEach (<anonymous>)
    at Object.<anonymous> (/home/runner/work/node/node/test/parallel/test-util-styletext.js:21:3)
    at Module._compile (node:internal/modules/cjs/loader:1368:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1426:10)
    at Module.load (node:internal/modules/cjs/loader:1205:32)
    at Module._load (node:internal/modules/cjs/loader:1021:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:142:12)
    at node:internal/main/run_main_module:28:49 {
  generatedMessage: false,
  code: 'ERR_ASSERTION',
  actual: undefined,
  expected: { code: 'ERR_INVALID_ARG_VALUE' },
  operator: 'throws'
}

Node.js v22.0.0-pre
Command: out/Release/node --test-reporter=spec --test-reporter-destination=stdout --test-reporter=./tools/github_reporter/index.js --test-reporter-destination=stdout /home/runner/work/node/node/test/parallel/test-util-styletext.js
assert.throws(() => {
util.styleText(invalidOption, 'test');
}, {
Expand All @@ -32,4 +37,27 @@
code: 'ERR_INVALID_ARG_VALUE',
});

assert.strictEqual(util.styleText('red', 'test'), '\u001b[31mtest\u001b[39m');
assert.strictEqual(util.styleText('red', 'test'), styled);

function checkCase(isTTY, env, expected) {
process.stdout.isTTY = isTTY;
const code = `
process.stdout.isTTY = ${isTTY}; console.log(util.styleText('red', 'test'));
`;
const output = execSync(`${process.execPath} -e "${code}"`, { env }).toString();
assert.strictEqual(output, expected + EOL);
}


[
{ isTTY: true, env: {}, expected: styled },
{ isTTY: false, env: {}, expected: noChange },
{ isTTY: true, env: { NODE_DISABLE_COLORS: 1 }, expected: noChange },
{ isTTY: true, env: { NO_COLOR: 1 }, expected: noChange },
{ isTTY: true, env: { FORCE_COLOR: 1 }, expected: styled },
{ isTTY: true, env: { FORCE_COLOR: 1, NODE_DISABLE_COLORS: 1 }, expected: styled },
{ isTTY: false, env: { FORCE_COLOR: 1, NO_COLOR: 1, NODE_DISABLE_COLORS: 1 }, expected: noChange },
{ isTTY: true, env: { FORCE_COLOR: 1, NO_COLOR: 1, NODE_DISABLE_COLORS: 1 }, expected: styled },
].forEach((testCase) => {
checkCase(testCase.isTTY, testCase.env, testCase.expected);
});
Loading