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: nodejs#33461 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax · Jun 25, 2020 · b831b08 · b831b08
1 parent 40bc309
commit b831b08
Show file tree

Hide file tree

Showing 12 changed files with 73 additions and 109 deletions.
diff --git 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
@@ -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
@@ -3,51 +3,35 @@
 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 {
     opts.historySize = 1000;
   }
 
   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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -35,22 +35,21 @@ 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(
     fileMode, 0o600,
     `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
@@ -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
@@ -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,15 +210,16 @@ function runTest(assertCleaned) {
     }
   }
 
-  const env = opts.env;
   const test = opts.test;
   const expected = opts.expected;
   const clean = opts.clean;
   const before = opts.before;
 
   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
@@ -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();