util: support array of formats in util.styleText

[marco-ippolito]

Fixes: #52035

[rauschma]

Looking at the code:

if (ArrayIsArray(format) && format.length > 0) {

What happens if the Array is empty? Maybe add a unit test for that.

[marco-ippolito]

Looking at the code:

if (ArrayIsArray(format) && format.length > 0) {

What happens if the Array is empty? Maybe add a unit test for that.

it's already present before:

node/test/parallel/test-util-styletext.js

Line 15 in 5984578

[],

[nodejs-github-bot]

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

[marco-ippolito]

I'm only not sure of the order in which we should add/remove styles:

For:

util.styleText(['red', 'bold'], 'test')

\u001b[31m: color to red.
\u001b[1m: style to bold.
test: The actual text content.
\u001b[39m: resets color.
\u001b[22m: removes bold.

OR

\u001b[1m: style to bold.
\u001b[31m: color to red.
test: The actual text content.
\u001b[39m: resets color.
\u001b[22m: removes bold.

Probably second one is more correct?

[rauschma]

I was asking because of this (I don’t know if that is intended or not):

if (ArrayIsArray(format) && format.length > 0) {
  // ···
  return `${formatCodes}${text}${append}`;
}
// ❌ `format` might be the empty Array here
const formatCodes = inspect.colors[format];
if (formatCodes == null) {
  validateOneOf(format, 'format', ObjectKeys(inspect.colors));
  // ···
}

[marco-ippolito]

I was asking because of this (I don’t know if that is intended or not):

if (ArrayIsArray(format) && format.length > 0) {
  // ···
  return `${formatCodes}${text}${append}`;
}
// ❌ `format` might be the empty Array here
const formatCodes = inspect.colors[format];
if (formatCodes == null) {
  validateOneOf(format, 'format', ObjectKeys(inspect.colors));
  // ···
}

If the array is empty it will throw ERR_INVALID_ARG_VALUE, it was already covered before my implementation and test

[rauschma]

Probably second one is more correct?

I’d say the following two invocations should produce the same result:

util.styleText('bold', util.styleText('red', 'Bold and red!'))
util.styleText(['bold', 'red'], 'Bold and red!')

[marco-ippolito]

Probably second one is more correct?

I’d say the following two invocations should produce the same result:

util.styleText('bold', util.styleText('red', 'Bold and red!')),
util.styleText(['bold', 'red'], 'Bold and red!'),

agreed, added a test to verify

[rauschma]

Another option is that an empty Array simply does nothing. Use case:

const styles = [];
if (warning) {
  styles.push('red');
}
console.log(util.styleText(styles, message));

[nodejs-github-bot]

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

[IlyasShabi]

LGTM

[rauschma]

Just in case: You can chain multiple codes via ; – which saves you a few characters in the Array case. But that may be too much effort for something that’s less frequently used.

This could look as follows (code may contain bugs – I haven’t tested it and am a bit tired):

/**
 * @param {number[]} codes
 * @returns {string}
 */
function escapeStyleCodes(codes) {
  return `\x1B[` + codes.join(';') + `m`;
}

/**
 * @param {string | string[]} format
 * @param {string} text
 * @returns {string}
 */
function styleText(format, text) {
  validateString(text, 'text');
  if (ArrayIsArray(format)) {
    const startCodes = [];
    const endCodes = [];
    for (const key of format) {
      const formatCodes = inspect.colors[key];
      if (formatCodes == null) {
        validateOneOf(key, 'format', ObjectKeys(inspect.colors));
      }
      startCodes.push(formatCodes[0]);
      endCodes.unshift(formatCodes[1]);
    }
    return escapeStyleCodes(startCodes) + text + escapeStyleCodes(endCodes);
  }

  const formatCodes = inspect.colors[format];
  if (formatCodes == null) {
    validateOneOf(format, 'format', ObjectKeys(inspect.colors));
  }
  return escapeStyleCodes([formatCodes[0]]) + text + escapeStyleCodes([formatCodes[1]]);
}

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 9a1e01c