assert,tty: fix indicator and warning #31429

BridgeAR · 2020-01-20T21:33:31Z

Please have a look at the commit messages. I can split this into two PRs but I stumbled upon both things at the same time and they seemed small enough that individual commits might suffice.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

This makes sure color codes are not taken into account in case util.inspect's default value was changed.

Make sure the assertion is actually triggered by using `assert.throws()` instead of `try/catch`.

It was possible that this warning ends up in an infinite recursion. The reason is that printing the warning triggered a color check and that triggered another warning. Limiting it to a single warning prevents this.

test/pseudo-tty/test-tty-color-support.out

nodejs-github-bot · 2020-01-25T09:42:42Z

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

nodejs-github-bot · 2020-01-26T18:24:16Z

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

nodejs-github-bot · 2020-01-26T19:01:01Z

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

BridgeAR · 2020-02-09T11:32:58Z

This could use another review. @nodejs/assert @nodejs/testing @nodejs/tty PTAL

BridgeAR · 2020-02-09T12:40:42Z

Landed in 1450ea7...63f10b9 🎉

This makes sure color codes are not taken into account in case util.inspect's default value was changed. PR-URL: nodejs#31429 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Make sure the assertion is actually triggered by using `assert.throws()` instead of `try/catch`. PR-URL: nodejs#31429 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

It was possible that this warning ends up in an infinite recursion. The reason is that printing the warning triggered a color check and that triggered another warning. Limiting it to a single warning prevents this. PR-URL: nodejs#31429 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

This makes sure color codes are not taken into account in case util.inspect's default value was changed. PR-URL: #31429 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Make sure the assertion is actually triggered by using `assert.throws()` instead of `try/catch`. PR-URL: #31429 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

It was possible that this warning ends up in an infinite recursion. The reason is that printing the warning triggered a color check and that triggered another warning. Limiting it to a single warning prevents this. PR-URL: #31429 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

vladimyr · 2020-02-29T01:56:40Z

@BridgeAR How do you feel about tweaking it a bit for sake of readability?

I guess it is slightly less performant but if you are fine with tradeoff I can do a quick PR with proposed changes.

diff --git a/lib/internal/tty.js b/lib/internal/tty.js
index 44d5173700..3e0da04890 100644
--- a/lib/internal/tty.js
+++ b/lib/internal/tty.js
@@ -26,6 +26,7 @@ const {
   ERR_INVALID_ARG_TYPE,
   ERR_OUT_OF_RANGE
 } = require('internal/errors').codes;
+const { Boolean } = primordials;
 
 let OSRelease;
 
@@ -77,23 +78,21 @@ let warned = false;
 function warnOnDeactivatedColors(env) {
   if (warned)
     return;
-  let name = '';
-  if (env.NODE_DISABLE_COLORS !== undefined)
-    name = 'NODE_DISABLE_COLORS';
-  if (env.NO_COLOR !== undefined) {
-    if (name !== '') {
-      name += "' and '";
-    }
-    name += 'NO_COLOR';
-  }
 
-  if (name !== '') {
-    process.emitWarning(
-      `The '${name}' env is ignored due to the 'FORCE_COLOR' env being set.`,
-      'Warning'
-    );
-    warned = true;
-  }
+  const name = [
+    env.NODE_DISABLE_COLORS !== undefined && '\'NODE_DISABLE_COLORS\'',
+    env.NO_COLOR !== undefined && '\'NO_COLOR\''
+  ].filter(Boolean).join(' and ');
+
+  if (!name)
+    return;
+
+  process.emitWarning(
+    `The ${name} env variable is ignored due to the 'FORCE_COLOR' env ` +
+    'variable being set.',
+    'Warning'
+  );
+  warned = true;
 }
 
 // The `getColorDepth` API got inspired by multiple sources such as