repl: fix terminal default setting and improve color check

doc/api/cli.md

@@ -671,7 +671,7 @@ added: v0.1.32
 added: v0.3.0
 -->
 
-When set to `1` colors will not be used in the REPL.
+When set, colors will not be used in the REPL.
 
 ### `NODE_EXTRA_CA_CERTS=file`
 <!-- YAML

doc/api/repl.md

@@ -479,6 +479,10 @@ with REPL instances programmatically.
 <!-- YAML
 added: v0.1.91
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/REPLACEME
+    description: The `terminal` option now follows the default description in
+                 all cases and `useColors` checks `hasColors()` if available.
   - version: v10.0.0
     pr-url: https://github.com/nodejs/node/pull/19187
     description: The `REPL_MAGIC_MODE` `replMode` was removed.
@@ -495,7 +499,7 @@ changes:
   * `output` {stream.Writable} The `Writable` stream to which REPL output will
     be written. **Default:** `process.stdout`.
   * `terminal` {boolean} If `true`, specifies that the `output` should be
-    treated as a TTY terminal, and have ANSI/VT100 escape codes written to it.
+    treated as a TTY terminal.
     **Default:** checking the value of the `isTTY` property on the `output`
     stream upon instantiation.
   * `eval` {Function} The function to be used when evaluating each given line
@@ -504,8 +508,9 @@ changes:
     the input was incomplete and prompt for additional lines.
   * `useColors` {boolean} If `true`, specifies that the default `writer`
     function should include ANSI color styling to REPL output. If a custom
-    `writer` function is provided then this has no effect. **Default:** the
-     REPL instances `terminal` value.
+    `writer` function is provided then this has no effect. **Default:** checking
+    color support on the `output` stream if the REPL instance's `terminal` value
+    is `true`.
   * `useGlobal` {boolean} If `true`, specifies that the default evaluation
      function will use the JavaScript `global` as the context as opposed to
      creating a new separate context for the REPL instance. The node CLI REPL

lib/internal/repl.js

@@ -14,7 +14,6 @@ function createRepl(env, opts, cb) {
   opts = {
     [kStandaloneREPL]: true,
     ignoreUndefined: false,
-    terminal: process.stdout.isTTY,
     useGlobal: true,
     breakEvalOnSigint: true,
     ...opts
@@ -23,17 +22,13 @@ function createRepl(env, opts, cb) {
   if (parseInt(env.NODE_NO_READLINE)) {
     opts.terminal = false;
   }
-  // The "dumb" special terminal, as defined by terminfo, doesn't support
-  // ANSI color control codes.
-  // see http://invisible-island.net/ncurses/terminfo.ti.html#toc-_Specials
-  if (parseInt(env.NODE_DISABLE_COLORS) || env.TERM === 'dumb') {
-    opts.useColors = false;
-  }
 
-  opts.replMode = {
-    'strict': REPL.REPL_MODE_STRICT,
-    'sloppy': REPL.REPL_MODE_SLOPPY
-  }[String(env.NODE_REPL_MODE).toLowerCase().trim()];
+  if (env.NODE_REPL_MODE) {
+    opts.replMode = {
+      'strict': REPL.REPL_MODE_STRICT,
+      'sloppy': REPL.REPL_MODE_SLOPPY
+    }[env.NODE_REPL_MODE.toLowerCase().trim()];
+  }
 
   if (opts.replMode === undefined) {
     opts.replMode = REPL.REPL_MODE_SLOPPY;
@@ -47,5 +42,6 @@ function createRepl(env, opts, cb) {
   }
 
   const repl = REPL.start(opts);
-  repl.setupHistory(opts.terminal ? env.NODE_REPL_HISTORY : '', cb);
+  const term = 'terminal' in opts ? opts.terminal : process.stdout.isTTY;
+  repl.setupHistory(term ? env.NODE_REPL_HISTORY : '', cb);
 }

lib/internal/tty.js

@@ -114,6 +114,9 @@ function getColorDepth(env = process.env) {
   if (env.NODE_DISABLE_COLORS !== undefined ||
       // See https://no-color.org/
       env.NO_COLOR !== undefined ||
+      // The "dumb" special terminal, as defined by terminfo, doesn't support
+      // ANSI color control codes.
+      // See http://invisible-island.net/ncurses/terminfo.ti.html#toc-_Specials
       env.TERM === 'dumb') {
     return COLORS_2;
   }

lib/repl.js

@@ -140,47 +140,70 @@ function REPLServer(prompt,
                           replMode);
   }
 
-  var options, input, output, dom, breakEvalOnSigint;
+  let options;
   if (prompt !== null && typeof prompt === 'object') {
-    // an options object was given
-    options = prompt;
+    // An options object was given.
+    options = { ...prompt };
     stream = options.stream || options.socket;
-    input = options.input;
-    output = options.output;
     eval_ = options.eval;
     useGlobal = options.useGlobal;
     ignoreUndefined = options.ignoreUndefined;
     prompt = options.prompt;
-    dom = options.domain;
     replMode = options.replMode;
-    breakEvalOnSigint = options.breakEvalOnSigint;
   } else {
     options = {};
   }
 
-  if (breakEvalOnSigint && eval_) {
+  if (!options.input && !options.output) {
+    // Legacy API, passing a 'stream'/'socket' option.
+    if (!stream) {
+      // Use stdin and stdout as the default streams if none were given.
+      stream = process;
+    }
+    // We're given a duplex readable/writable Stream, like a `net.Socket`
+    // or a custom object with 2 streams, or the `process` object.
+    options.input = stream.stdin || stream;
+    options.output = stream.stdout || stream;
+  }
+
+  if (options.terminal === undefined) {
+    options.terminal = options.output.isTTY;
+  }
+  options.terminal = !!options.terminal;
+
+  if (options.terminal && options.useColors === undefined) {
+    // If possible, check if stdout supports colors or not.
+    if (options.output.hasColors) {
+      options.useColors = options.output.hasColors();
+    } else if (process.env.NODE_DISABLE_COLORS === undefined) {
+      options.useColors = true;
+    }
+  }
+
+  this.inputStream = options.input;
+  this.outputStream = options.output;
+  this.useColors = !!options.useColors;
+  this._domain = options.domain || domain.create();
+  this.useGlobal = !!useGlobal;
+  this.ignoreUndefined = !!ignoreUndefined;
+  this.replMode = replMode || exports.REPL_MODE_SLOPPY;
+  this.underscoreAssigned = false;
+  this.last = undefined;
+  this.underscoreErrAssigned = false;
+  this.lastError = undefined;
+  this.breakEvalOnSigint = !!options.breakEvalOnSigint;
+  this.editorMode = false;
+  // Context id for use with the inspector protocol.
+  this[kContextId] = undefined;
+
+  if (this.breakEvalOnSigint && eval_) {
     // Allowing this would not reflect user expectations.
     // breakEvalOnSigint affects only the behavior of the default eval().
     throw new ERR_INVALID_REPL_EVAL_CONFIG();
   }
 
-  var self = this;
-
-  self._domain = dom || domain.create();
-  self.useGlobal = !!useGlobal;
-  self.ignoreUndefined = !!ignoreUndefined;
-  self.replMode = replMode || exports.REPL_MODE_SLOPPY;
-  self.underscoreAssigned = false;
-  self.last = undefined;
-  self.underscoreErrAssigned = false;
-  self.lastError = undefined;
-  self.breakEvalOnSigint = !!breakEvalOnSigint;
-  self.editorMode = false;
-  // Context id for use with the inspector protocol.
-  self[kContextId] = undefined;
-
-  let rli = self;
-  Object.defineProperty(self, 'rli', {
+  let rli = this;
+  Object.defineProperty(this, 'rli', {
     get: util.deprecate(() => rli,
                         'REPLServer.rli is deprecated', 'DEP0124'),
     set: util.deprecate((val) => rli = val,
@@ -197,6 +220,8 @@ function REPLServer(prompt,
 
   eval_ = eval_ || defaultEval;
 
+  const self = this;
+
   // Pause taking in new input, and store the keys in a buffer.
   const pausedBuffer = [];
   let paused = false;
@@ -452,21 +477,6 @@ function REPLServer(prompt,
     top.displayPrompt();
   });
 
-  if (!input && !output) {
-    // legacy API, passing a 'stream'/'socket' option
-    if (!stream) {
-      // Use stdin and stdout as the default streams if none were given
-      stream = process;
-    }
-    // We're given a duplex readable/writable Stream, like a `net.Socket`
-    // or a custom object with 2 streams, or the `process` object
-    input = stream.stdin || stream;
-    output = stream.stdout || stream;
-  }
-
-  self.inputStream = input;
-  self.outputStream = output;
-
   self.resetContext();
   self.lines.level = [];
 
@@ -503,13 +513,6 @@ function REPLServer(prompt,
   // Figure out which "writer" function to use
   self.writer = options.writer || exports.writer;
 
-  if (options.useColors === undefined) {
-    options.useColors = self.terminal && (
-      typeof self.outputStream.getColorDepth === 'function' ?
-        self.outputStream.getColorDepth() > 1 : true);
-  }
-  self.useColors = !!options.useColors;
-
   if (self.writer === writer) {
     // Conditionally turn on ANSI coloring.
     writer.options.colors = self.useColors;

test/parallel/test-repl-colors.js

@@ -19,6 +19,8 @@ inout._write = function(s, _, cb) {
 };
 
 const repl = new REPLServer({ input: inout, output: inout, useColors: true });
+inout.isTTY = true;
+const repl2 = new REPLServer({ input: inout, output: inout });
 
 process.on('exit', function() {
   // https://github.com/nodejs/node/pull/16485#issuecomment-350428638
@@ -28,4 +30,5 @@ process.on('exit', function() {
   strictEqual(output.includes(`'\u001b[32m\\'string\\'\u001b[39m'`), false);
   strictEqual(inspect.defaultOptions.colors, false);
   strictEqual(repl.writer.options.colors, true);
+  strictEqual(repl2.writer.options.colors, true);
 });

test/parallel/test-repl-envvars.js

@@ -38,19 +38,25 @@ const tests = [
 function run(test) {
   const env = test.env;
   const expected = test.expected;
+
   const opts = {
     terminal: true,
     input: new stream.Readable({ read() {} }),
     output: new stream.Writable({ write() {} })
   };
 
-  REPL.createInternalRepl(env, opts, function(err, repl) {
+  Object.assign(process.env, env);
+
+  REPL.createInternalRepl(process.env, opts, function(err, repl) {
     assert.ifError(err);
 
     assert.strictEqual(repl.terminal, expected.terminal,
                        `Expected ${inspect(expected)} with ${inspect(env)}`);
     assert.strictEqual(repl.useColors, expected.useColors,
                        `Expected ${inspect(expected)} with ${inspect(env)}`);
+    for (const key of Object.keys(env)) {
+      delete process.env[key];
+    }
     repl.close();
   });
 }