From 740c6f1ade84b3189987ec0e6011496cbff490b7 Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Fri, 8 Mar 2019 12:21:35 +0100
Subject: [PATCH] repl: fix terminal default setting

This makes sure that the described default behavior for the
`terminal` option is actually always used and not only when running
the REPL as standalone program.

The options code is now logically combined instead of being spread
out in the big REPL constructor.
---
 doc/api/cli.md                     |  2 +-
 doc/api/repl.md                    | 11 +++-
 lib/internal/repl.js               | 20 +++---
 lib/internal/tty.js                |  3 +
 lib/repl.js                        | 97 +++++++++++++++---------------
 test/parallel/test-repl-colors.js  |  3 +
 test/parallel/test-repl-envvars.js |  8 ++-
 7 files changed, 80 insertions(+), 64 deletions(-)

diff --git a/doc/api/cli.md b/doc/api/cli.md
index cf28d9b2bf9e64..75804e11521aee 100644
--- a/doc/api/cli.md
+++ b/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
diff --git a/doc/api/repl.md b/doc/api/repl.md
index 4ea2e8c9063d0a..cc7f3f96da15d7 100644
--- a/doc/api/repl.md
+++ b/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
diff --git a/lib/internal/repl.js b/lib/internal/repl.js
index d84031fc77ce6c..7ef14d33b341fe 100644
--- a/lib/internal/repl.js
+++ b/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);
 }
diff --git a/lib/internal/tty.js b/lib/internal/tty.js
index 9c34d3fdb7f950..fc7bde724552d5 100644
--- a/lib/internal/tty.js
+++ b/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;
   }
diff --git a/lib/repl.js b/lib/repl.js
index 7e186778a48f8c..ea4f6ac7e79c52 100644
--- a/lib/repl.js
+++ b/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;
diff --git a/test/parallel/test-repl-colors.js b/test/parallel/test-repl-colors.js
index f484f57945a16b..dd1bdb1a08ee9e 100644
--- a/test/parallel/test-repl-colors.js
+++ b/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);
 });
diff --git a/test/parallel/test-repl-envvars.js b/test/parallel/test-repl-envvars.js
index 6679cd6cd73a31..4fab77b7c09980 100644
--- a/test/parallel/test-repl-envvars.js
+++ b/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();
   });
 }