Revert "repl: always check for NODE_REPL_MODE environment variable"

This reverts commit b831b08. This presumably unbreaks the ASAN github action build. Example failure: 2020-06-25T19:59:15.1448178Z === release test-repl-envvars === 2020-06-25T19:59:15.1448872Z Path: parallel/test-repl-envvars 2020-06-25T19:59:15.1449449Z --- stderr --- 2020-06-25T19:59:15.1449835Z assert.js:103 2020-06-25T19:59:15.1450194Z throw new AssertionError(obj); 2020-06-25T19:59:15.1450524Z ^ 2020-06-25T19:59:15.1450817Z 2020-06-25T19:59:15.1451431Z AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal: 2020-06-25T19:59:15.1452000Z + actual - expected 2020-06-25T19:59:15.1452298Z 2020-06-25T19:59:15.1452634Z { 2020-06-25T19:59:15.1452978Z terminal: true, 2020-06-25T19:59:15.1453321Z + useColors: false 2020-06-25T19:59:15.1453861Z - useColors: true 2020-06-25T19:59:15.1454225Z } 2020-06-25T19:59:15.1454841Z at /home/runner/work/node/node/test/parallel/test-repl-envvars.js:55:12 2020-06-25T19:59:15.1455246Z at internal/repl.js:33:5 PR-URL: nodejs#34058 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]>
Trott · Jun 26, 2020 · 072feec · 072feec
1 parent 82f13fa
commit 072feec
Show file tree

Hide file tree

Showing 12 changed files with 109 additions and 73 deletions.
diff --git a/doc/api/repl.md b/doc/api/repl.md
@@ -555,10 +555,6 @@ 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,7 +35,21 @@ if (process.env.NODE_REPL_EXTERNAL_MODULE) {
   console.log(`Welcome to Node.js ${process.version}.\n` +
     'Type ".help" for more information.');
 
-  require('internal/repl').createInternalRepl();
+  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();
+    });
+  });
 
   // 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,35 +3,51 @@
 const {
   Number,
   NumberIsNaN,
+  ObjectCreate,
 } = primordials;
 
 const REPL = require('repl');
 const { kStandaloneREPL } = require('internal/repl/utils');
 
-function createInternalRepl(opts = {}, callback = () => {}) {
+module.exports = ObjectCreate(REPL);
+module.exports.createInternalRepl = createRepl;
+
+function createRepl(env, opts, cb) {
+  if (typeof opts === 'function') {
+    cb = opts;
+    opts = null;
+  }
   opts = {
     [kStandaloneREPL]: true,
+    ignoreUndefined: false,
     useGlobal: true,
     breakEvalOnSigint: true,
-    ...opts,
-    terminal: parseInt(process.env.NODE_NO_READLINE) ? false : opts.terminal,
+    ...opts
   };
 
-  const historySize = Number(process.env.NODE_REPL_HISTORY_SIZE);
+  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);
   if (!NumberIsNaN(historySize) && historySize > 0) {
     opts.historySize = historySize;
   } else {
     opts.historySize = 1000;
   }
 
   const repl = REPL.start(opts);
-  const historyPath = repl.terminal ? process.env.NODE_REPL_HISTORY : '';
-  repl.setupHistory(historyPath, (err, repl) => {
-    if (err) {
-      throw err;
-    }
-    callback(repl);
-  });
+  const term = 'terminal' in opts ? opts.terminal : process.stdout.isTTY;
+  repl.setupHistory(term ? env.NODE_REPL_HISTORY : '', cb);
 }
-
-module.exports = { createInternalRepl };
diff --git a/lib/repl.js b/lib/repl.js
@@ -115,7 +115,6 @@ const {
   setupPreview,
   setupReverseSearch,
 } = require('internal/repl/utils');
-const { hasColors } = require('internal/tty');
 const {
   getOwnNonIndexProperties,
   propertyFilter: {
@@ -214,8 +213,8 @@ function REPLServer(prompt,
     // If possible, check if stdout supports colors or not.
     if (options.output.hasColors) {
       options.useColors = options.output.hasColors();
-    } else {
-      options.useColors = hasColors();
+    } else if (process.env.NODE_DISABLE_COLORS === undefined) {
+      options.useColors = true;
     }
   }
 
@@ -260,6 +259,7 @@ 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,11 +269,6 @@ 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,7 +20,6 @@ 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 debug = require('util').debuglog('test');
+const inspect = require('util').inspect;
 
 const tests = [
   {
-    env: { TERM: 'xterm-256' },
+    env: {},
     expected: { terminal: true, useColors: true }
   },
   {
@@ -30,33 +30,35 @@ const tests = [
     expected: { terminal: false, useColors: false }
   },
   {
-    env: { NODE_NO_READLINE: '0', TERM: 'xterm-256' },
+    env: { NODE_NO_READLINE: '0' },
     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() {} })
   };
 
-  REPL.createInternalRepl(opts, (repl) => {
-    debug(env);
-    const { terminal, useColors } = repl;
-    assert.deepStrictEqual({ terminal, useColors }, expected);
+  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();
-    if (tests.length)
-      run(tests.shift());
   });
 }
 
-run(tests.shift());
+tests.forEach(run);
diff --git a/test/parallel/test-repl-history-navigation.js b/test/parallel/test-repl-history-navigation.js
@@ -521,8 +521,6 @@ function cleanupTmpFile() {
   return true;
 }
 
-const originalEnv = process.env;
-
 function runTest() {
   const opts = tests.shift();
   if (!opts) return; // All done
@@ -537,9 +535,7 @@ function runTest() {
   const lastChunks = [];
   let i = 0;
 
-  process.env = { ...originalEnv, ...opts.env };
-
-  REPL.createInternalRepl({
+  REPL.createInternalRepl(opts.env, {
     input: new ActionStream(),
     output: new stream.Writable({
       write(chunk, _, next) {
@@ -574,7 +570,12 @@ function runTest() {
     useColors: false,
     preview: opts.preview,
     terminal: true
-  }, function(repl) {
+  }, function(err, repl) {
+    if (err) {
+      console.error(`Failed test # ${numtests - tests.length}`);
+      throw err;
+    }
+
     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,21 +35,22 @@ const tmpdir = require('../common/tmpdir');
 tmpdir.refresh();
 const replHistoryPath = path.join(tmpdir.path, '.node_repl_history');
 
-const checkResults = common.mustCall((repl) => {
+const checkResults = common.mustCall(function(err, r) {
+  assert.ifError(err);
+
   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
-  repl.input.emit('keypress', '', { ctrl: true, name: 'd' });
-  repl.input.end();
+  r.input.emit('keypress', '', { ctrl: true, name: 'd' });
+  r.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,7 +42,6 @@ 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,8 +191,6 @@ 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
@@ -210,16 +208,15 @@ 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();
 
-  process.env = { ...originalEnv, ...opts.env };
-
-  REPL.createInternalRepl({
+  REPL.createInternalRepl(env, {
     input: new ActionStream(),
     output: new stream.Writable({
       write(chunk, _, next) {
@@ -242,7 +239,12 @@ function runTest(assertCleaned) {
     prompt,
     useColors: false,
     terminal: true
-  }, function(repl) {
+  }, function(err, repl) {
+    if (err) {
+      console.error(`Failed test # ${numtests - tests.length}`);
+      throw err;
+    }
+
     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,9 +282,6 @@ function cleanupTmpFile() {
   return true;
 }
 
-
-const originalEnv = { ...process.env };
-
 function runTest() {
   const opts = tests.shift();
   if (!opts) return; // All done
@@ -300,9 +297,7 @@ function runTest() {
   const lastChunks = [];
   let i = 0;
 
-  process.env = { ...originalEnv, ...opts.env };
-
-  REPL.createInternalRepl({
+  REPL.createInternalRepl(opts.env, {
     input: new ActionStream(),
     output: new stream.Writable({
       write(chunk, _, next) {
@@ -335,7 +330,12 @@ function runTest() {
     prompt,
     useColors: opts.useColors || false,
     terminal: true
-  }, function(repl) {
+  }, function(err, repl) {
+    if (err) {
+      console.error(`Failed test # ${numtests - tests.length}`);
+      throw err;
+    }
+
     repl.once('close', () => {
       if (opts.clean)
         cleanupTmpFile();