From b831b081c499a614c1ee3f0272c9de1783395402 Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Tue, 19 May 2020 02:33:55 +0200
Subject: [PATCH] repl: always check for NODE_REPL_MODE environment variable

This makes sure all REPL instances check for the NODE_REPL_MODE
environment variable in case the `replMode` is not passed through
as option.

At the same time this simplifies the internal REPL code significantly.

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: https://github.com/nodejs/node/pull/33461
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
---
 doc/api/repl.md                               |  4 ++
 lib/internal/main/repl.js                     | 16 +------
 lib/internal/repl.js                          | 42 ++++++-------------
 lib/repl.js                                   | 11 +++--
 test/parallel/test-repl-colors.js             |  1 +
 test/parallel/test-repl-envvars.js            | 30 +++++++------
 test/parallel/test-repl-history-navigation.js | 13 +++---
 test/parallel/test-repl-history-perm.js       | 11 +++--
 test/parallel/test-repl-options.js            |  1 +
 test/parallel/test-repl-persistent-history.js | 14 +++----
 test/parallel/test-repl-reverse-search.js     | 14 +++----
 test/parallel/test-repl-use-global.js         | 25 ++++-------
 12 files changed, 73 insertions(+), 109 deletions(-)

diff --git a/doc/api/repl.md b/doc/api/repl.md
index 614c577d2cfd44..05b8851b8eff94 100644
--- a/doc/api/repl.md
+++ b/doc/api/repl.md
@@ -555,6 +555,10 @@ A list of the names of all Node.js modules, e.g., `'http'`.
 <!-- YAML
 added: v0.1.91
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/33461
+    description: The `NODE_REPL_MODE` environment variable now accounts for
+                 all repl instances.
   - version:
      - v13.4.0
      - v12.17.0
diff --git a/lib/internal/main/repl.js b/lib/internal/main/repl.js
index 693b7048a37f9c..55a88a2b181e98 100644
--- a/lib/internal/main/repl.js
+++ b/lib/internal/main/repl.js
@@ -35,21 +35,7 @@ if (process.env.NODE_REPL_EXTERNAL_MODULE) {
   console.log(`Welcome to Node.js ${process.version}.\n` +
     'Type ".help" for more information.');
 
-  const cliRepl = require('internal/repl');
-  cliRepl.createInternalRepl(process.env, (err, repl) => {
-    if (err) {
-      throw err;
-    }
-    repl.on('exit', () => {
-      if (repl._flushing) {
-        repl.pause();
-        return repl.once('flushHistory', () => {
-          process.exit();
-        });
-      }
-      process.exit();
-    });
-  });
+  require('internal/repl').createInternalRepl();
 
   // If user passed '-e' or '--eval' along with `-i` or `--interactive`,
   // evaluate the code in the current context.
diff --git a/lib/internal/repl.js b/lib/internal/repl.js
index 565ab049c71487..ca99d1f734cb26 100644
--- a/lib/internal/repl.js
+++ b/lib/internal/repl.js
@@ -3,44 +3,21 @@
 const {
   Number,
   NumberIsNaN,
-  ObjectCreate,
 } = primordials;
 
 const REPL = require('repl');
 const { kStandaloneREPL } = require('internal/repl/utils');
 
-module.exports = ObjectCreate(REPL);
-module.exports.createInternalRepl = createRepl;
-
-function createRepl(env, opts, cb) {
-  if (typeof opts === 'function') {
-    cb = opts;
-    opts = null;
-  }
+function createInternalRepl(opts = {}, callback = () => {}) {
   opts = {
     [kStandaloneREPL]: true,
-    ignoreUndefined: false,
     useGlobal: true,
     breakEvalOnSigint: true,
-    ...opts
+    ...opts,
+    terminal: parseInt(process.env.NODE_NO_READLINE) ? false : opts.terminal,
   };
 
-  if (parseInt(env.NODE_NO_READLINE)) {
-    opts.terminal = false;
-  }
-
-  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;
-  }
-
-  const historySize = Number(env.NODE_REPL_HISTORY_SIZE);
+  const historySize = Number(process.env.NODE_REPL_HISTORY_SIZE);
   if (!NumberIsNaN(historySize) && historySize > 0) {
     opts.historySize = historySize;
   } else {
@@ -48,6 +25,13 @@ function createRepl(env, opts, cb) {
   }
 
   const repl = REPL.start(opts);
-  const term = 'terminal' in opts ? opts.terminal : process.stdout.isTTY;
-  repl.setupHistory(term ? env.NODE_REPL_HISTORY : '', cb);
+  const historyPath = repl.terminal ? process.env.NODE_REPL_HISTORY : '';
+  repl.setupHistory(historyPath, (err, repl) => {
+    if (err) {
+      throw err;
+    }
+    callback(repl);
+  });
 }
+
+module.exports = { createInternalRepl };
diff --git a/lib/repl.js b/lib/repl.js
index a755dbf1bb107e..3ffa0d006b42a0 100644
--- a/lib/repl.js
+++ b/lib/repl.js
@@ -115,6 +115,7 @@ const {
   setupPreview,
   setupReverseSearch,
 } = require('internal/repl/utils');
+const { hasColors } = require('internal/tty');
 const {
   getOwnNonIndexProperties,
   propertyFilter: {
@@ -213,8 +214,8 @@ function REPLServer(prompt,
     // 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;
+    } else {
+      options.useColors = hasColors();
     }
   }
 
@@ -259,7 +260,6 @@ function REPLServer(prompt,
   this._domain = options.domain || domain.create();
   this.useGlobal = !!useGlobal;
   this.ignoreUndefined = !!ignoreUndefined;
-  this.replMode = replMode || module.exports.REPL_MODE_SLOPPY;
   this.underscoreAssigned = false;
   this.last = undefined;
   this.underscoreErrAssigned = false;
@@ -269,6 +269,11 @@ function REPLServer(prompt,
   // Context id for use with the inspector protocol.
   this[kContextId] = undefined;
 
+  this.replMode = replMode ||
+    (/^\s*strict\s*$/i.test(process.env.NODE_REPL_MODE) ?
+      module.exports.REPL_MODE_STRICT :
+      module.exports.REPL_MODE_SLOPPY);
+
   if (this.breakEvalOnSigint && eval_) {
     // Allowing this would not reflect user expectations.
     // breakEvalOnSigint affects only the behavior of the default eval().
diff --git a/test/parallel/test-repl-colors.js b/test/parallel/test-repl-colors.js
index dd1bdb1a08ee9e..8b2394e0a435b0 100644
--- a/test/parallel/test-repl-colors.js
+++ b/test/parallel/test-repl-colors.js
@@ -20,6 +20,7 @@ inout._write = function(s, _, cb) {
 
 const repl = new REPLServer({ input: inout, output: inout, useColors: true });
 inout.isTTY = true;
+inout.hasColors = () => true;
 const repl2 = new REPLServer({ input: inout, output: inout });
 
 process.on('exit', function() {
diff --git a/test/parallel/test-repl-envvars.js b/test/parallel/test-repl-envvars.js
index 4fab77b7c09980..acd144b5bb978d 100644
--- a/test/parallel/test-repl-envvars.js
+++ b/test/parallel/test-repl-envvars.js
@@ -6,11 +6,11 @@ require('../common');
 const stream = require('stream');
 const REPL = require('internal/repl');
 const assert = require('assert');
-const inspect = require('util').inspect;
+const debug = require('util').debuglog('test');
 
 const tests = [
   {
-    env: {},
+    env: { TERM: 'xterm-256' },
     expected: { terminal: true, useColors: true }
   },
   {
@@ -30,35 +30,33 @@ const tests = [
     expected: { terminal: false, useColors: false }
   },
   {
-    env: { NODE_NO_READLINE: '0' },
+    env: { NODE_NO_READLINE: '0', TERM: 'xterm-256' },
     expected: { terminal: true, useColors: true }
   }
 ];
 
+const originalEnv = process.env;
+
 function run(test) {
   const env = test.env;
   const expected = test.expected;
 
+  process.env = { ...originalEnv, ...env };
+
   const opts = {
     terminal: true,
     input: new stream.Readable({ read() {} }),
     output: new stream.Writable({ write() {} })
   };
 
-  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.createInternalRepl(opts, (repl) => {
+    debug(env);
+    const { terminal, useColors } = repl;
+    assert.deepStrictEqual({ terminal, useColors }, expected);
     repl.close();
+    if (tests.length)
+      run(tests.shift());
   });
 }
 
-tests.forEach(run);
+run(tests.shift());
diff --git a/test/parallel/test-repl-history-navigation.js b/test/parallel/test-repl-history-navigation.js
index 9155ad722b8710..9d587df7bd4757 100644
--- a/test/parallel/test-repl-history-navigation.js
+++ b/test/parallel/test-repl-history-navigation.js
@@ -521,6 +521,8 @@ function cleanupTmpFile() {
   return true;
 }
 
+const originalEnv = process.env;
+
 function runTest() {
   const opts = tests.shift();
   if (!opts) return; // All done
@@ -535,7 +537,9 @@ function runTest() {
   const lastChunks = [];
   let i = 0;
 
-  REPL.createInternalRepl(opts.env, {
+  process.env = { ...originalEnv, ...opts.env };
+
+  REPL.createInternalRepl({
     input: new ActionStream(),
     output: new stream.Writable({
       write(chunk, _, next) {
@@ -570,12 +574,7 @@ function runTest() {
     useColors: false,
     preview: opts.preview,
     terminal: true
-  }, function(err, repl) {
-    if (err) {
-      console.error(`Failed test # ${numtests - tests.length}`);
-      throw err;
-    }
-
+  }, function(repl) {
     repl.once('close', () => {
       if (opts.clean)
         cleanupTmpFile();
diff --git a/test/parallel/test-repl-history-perm.js b/test/parallel/test-repl-history-perm.js
index 7297689422eba1..736b73206b43f5 100644
--- a/test/parallel/test-repl-history-perm.js
+++ b/test/parallel/test-repl-history-perm.js
@@ -35,9 +35,7 @@ const tmpdir = require('../common/tmpdir');
 tmpdir.refresh();
 const replHistoryPath = path.join(tmpdir.path, '.node_repl_history');
 
-const checkResults = common.mustCall(function(err, r) {
-  assert.ifError(err);
-
+const checkResults = common.mustCall((repl) => {
   const stat = fs.statSync(replHistoryPath);
   const fileMode = stat.mode & 0o777;
   assert.strictEqual(
@@ -45,12 +43,13 @@ const checkResults = common.mustCall(function(err, r) {
     `REPL history file should be mode 0600 but was 0${fileMode.toString(8)}`);
 
   // Close the REPL
-  r.input.emit('keypress', '', { ctrl: true, name: 'd' });
-  r.input.end();
+  repl.input.emit('keypress', '', { ctrl: true, name: 'd' });
+  repl.input.end();
 });
 
+process.env.NODE_REPL_HISTORY = replHistoryPath;
+
 repl.createInternalRepl(
-  { NODE_REPL_HISTORY: replHistoryPath },
   {
     terminal: true,
     input: stream,
diff --git a/test/parallel/test-repl-options.js b/test/parallel/test-repl-options.js
index 3f6e16ee47fba2..6a02b67917aa03 100644
--- a/test/parallel/test-repl-options.js
+++ b/test/parallel/test-repl-options.js
@@ -42,6 +42,7 @@ common.expectWarning({
 
 // Create a dummy stream that does nothing
 const stream = new ArrayStream();
+stream.hasColors = () => true;
 
 // 1, mostly defaults
 const r1 = repl.start({
diff --git a/test/parallel/test-repl-persistent-history.js b/test/parallel/test-repl-persistent-history.js
index 705a7c7c236fb0..0ba78c93a42ade 100644
--- a/test/parallel/test-repl-persistent-history.js
+++ b/test/parallel/test-repl-persistent-history.js
@@ -191,6 +191,8 @@ fs.createReadStream(historyFixturePath)
 
 const runTestWrap = common.mustCall(runTest, numtests);
 
+const originalEnv = process.env;
+
 function runTest(assertCleaned) {
   const opts = tests.shift();
   if (!opts) return; // All done
@@ -208,7 +210,6 @@ function runTest(assertCleaned) {
     }
   }
 
-  const env = opts.env;
   const test = opts.test;
   const expected = opts.expected;
   const clean = opts.clean;
@@ -216,7 +217,9 @@ function runTest(assertCleaned) {
 
   if (before) before();
 
-  REPL.createInternalRepl(env, {
+  process.env = { ...originalEnv, ...opts.env };
+
+  REPL.createInternalRepl({
     input: new ActionStream(),
     output: new stream.Writable({
       write(chunk, _, next) {
@@ -239,12 +242,7 @@ function runTest(assertCleaned) {
     prompt,
     useColors: false,
     terminal: true
-  }, function(err, repl) {
-    if (err) {
-      console.error(`Failed test # ${numtests - tests.length}`);
-      throw err;
-    }
-
+  }, function(repl) {
     repl.once('close', () => {
       if (repl._flushing) {
         repl.once('flushHistory', onClose);
diff --git a/test/parallel/test-repl-reverse-search.js b/test/parallel/test-repl-reverse-search.js
index 5027bd7da497fe..75364b0ffb1509 100644
--- a/test/parallel/test-repl-reverse-search.js
+++ b/test/parallel/test-repl-reverse-search.js
@@ -282,6 +282,9 @@ function cleanupTmpFile() {
   return true;
 }
 
+
+const originalEnv = { ...process.env };
+
 function runTest() {
   const opts = tests.shift();
   if (!opts) return; // All done
@@ -297,7 +300,9 @@ function runTest() {
   const lastChunks = [];
   let i = 0;
 
-  REPL.createInternalRepl(opts.env, {
+  process.env = { ...originalEnv, ...opts.env };
+
+  REPL.createInternalRepl({
     input: new ActionStream(),
     output: new stream.Writable({
       write(chunk, _, next) {
@@ -330,12 +335,7 @@ function runTest() {
     prompt,
     useColors: opts.useColors || false,
     terminal: true
-  }, function(err, repl) {
-    if (err) {
-      console.error(`Failed test # ${numtests - tests.length}`);
-      throw err;
-    }
-
+  }, function(repl) {
     repl.once('close', () => {
       if (opts.clean)
         cleanupTmpFile();
diff --git a/test/parallel/test-repl-use-global.js b/test/parallel/test-repl-use-global.js
index 00222608deca6f..f979b0c336e0b2 100644
--- a/test/parallel/test-repl-use-global.js
+++ b/test/parallel/test-repl-use-global.js
@@ -14,23 +14,19 @@ const globalTestCases = [
   [undefined, 'undefined']
 ];
 
-const globalTest = (useGlobal, cb, output) => (err, repl) => {
-  if (err)
-    return cb(err);
-
+const globalTest = (cb, output) => (repl) => {
   let str = '';
   output.on('data', (data) => (str += data));
   global.lunch = 'tacos';
   repl.write('global.lunch;\n');
   repl.close();
   delete global.lunch;
-  cb(null, str.trim());
+  cb(str.trim());
 };
 
 // Test how the global object behaves in each state for useGlobal
 for (const [option, expected] of globalTestCases) {
-  runRepl(option, globalTest, common.mustCall((err, output) => {
-    assert.ifError(err);
+  runRepl(option, globalTest, common.mustCall((output) => {
     assert.strictEqual(output, expected);
   }));
 }
@@ -43,10 +39,7 @@ for (const [option, expected] of globalTestCases) {
 // suite is aware of it, causing a failure to be flagged.
 //
 const processTestCases = [false, undefined];
-const processTest = (useGlobal, cb, output) => (err, repl) => {
-  if (err)
-    return cb(err);
-
+const processTest = (cb, output) => (repl) => {
   let str = '';
   output.on('data', (data) => (str += data));
 
@@ -54,12 +47,11 @@ const processTest = (useGlobal, cb, output) => (err, repl) => {
   repl.write('let process;\n');
   repl.write('21 * 2;\n');
   repl.close();
-  cb(null, str.trim());
+  cb(str.trim());
 };
 
 for (const option of processTestCases) {
-  runRepl(option, processTest, common.mustCall((err, output) => {
-    assert.ifError(err);
+  runRepl(option, processTest, common.mustCall((output) => {
     assert.strictEqual(output, 'undefined\n42');
   }));
 }
@@ -76,8 +68,5 @@ function runRepl(useGlobal, testFunc, cb) {
     prompt: ''
   };
 
-  repl.createInternalRepl(
-    process.env,
-    opts,
-    testFunc(useGlobal, cb, opts.output));
+  repl.createInternalRepl(opts, testFunc(cb, opts.output));
 }