repl: fix old history error handling

PR-URL: https://github.com/nodejs/node/pull/13733
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

doc/api/errors.md

@@ -683,12 +683,6 @@ passed in an options object.
 Used when both `breakEvalOnSigint` and `eval` options are set
 in the REPL config, which is not supported.
 
-<a id="ERR_INVALID_REPL_HISTORY"></a>
-### ERR_INVALID_REPL_HISTORY
-
-Used in the `repl` in case the old history file is used and an error occurred
-while trying to read and parse it.
-
 <a id="ERR_INVALID_SYNC_FORK_INPUT"></a>
 ### ERR_INVALID_SYNC_FORK_INPUT
 

lib/internal/errors.js

@@ -144,7 +144,6 @@ E('ERR_INVALID_OPT_VALUE',
   });
 E('ERR_INVALID_REPL_EVAL_CONFIG',
   'Cannot specify both "breakEvalOnSigint" and "eval" for REPL');
-E('ERR_INVALID_REPL_HISTORY', 'Expected array, got %s');
 E('ERR_INVALID_SYNC_FORK_INPUT',
   'Asynchronous forks do not support Buffer, Uint8Array or string input: %s');
 E('ERR_INVALID_THIS', 'Value of "this" must be of type %s');

lib/internal/repl.js

@@ -7,8 +7,6 @@ const fs = require('fs');
 const os = require('os');
 const util = require('util');
 const debug = util.debuglog('repl');
-const errors = require('internal/errors');
-
 module.exports = Object.create(REPL);
 module.exports.createInternalRepl = createRepl;
 
@@ -16,6 +14,11 @@ module.exports.createInternalRepl = createRepl;
 // The debounce is to guard against code pasted into the REPL.
 const kDebounceHistoryMS = 15;
 
+function _writeToOutput(repl, message) {
+  repl._writeToOutput(message);
+  repl._refreshLine();
+}
+
 function createRepl(env, opts, cb) {
   if (typeof opts === 'function') {
     cb = opts;
@@ -81,9 +84,8 @@ function setupHistory(repl, historyPath, oldHistoryPath, ready) {
     try {
       historyPath = path.join(os.homedir(), '.node_repl_history');
     } catch (err) {
-      repl._writeToOutput('\nError: Could not get the home directory.\n' +
+      _writeToOutput(repl, '\nError: Could not get the home directory.\n' +
-                          'REPL session history will not be persisted.\n');
+                     'REPL session history will not be persisted.\n');
-      repl._refreshLine();
 
       debug(err.stack);
       repl._historyPrev = _replHistoryMessage;
@@ -104,9 +106,8 @@ function setupHistory(repl, historyPath, oldHistoryPath, ready) {
     if (err) {
       // Cannot open history file.
       // Don't crash, just don't persist history.
-      repl._writeToOutput('\nError: Could not open history file.\n' +
+      _writeToOutput(repl, '\nError: Could not open history file.\n' +
-                          'REPL session history will not be persisted.\n');
+                           'REPL session history will not be persisted.\n');
-      repl._refreshLine();
       debug(err.stack);
 
       repl._historyPrev = _replHistoryMessage;
@@ -133,18 +134,13 @@ function setupHistory(repl, historyPath, oldHistoryPath, ready) {
     } else if (oldHistoryPath === historyPath) {
       // If pre-v3.0, the user had set NODE_REPL_HISTORY_FILE to
       // ~/.node_repl_history, warn the user about it and proceed.
-      repl._writeToOutput(
+      _writeToOutput(
-          '\nThe old repl history file has the same name and location as ' +
+        repl,
+        '\nThe old repl history file has the same name and location as ' +
           `the new one i.e., ${historyPath} and is empty.\nUsing it as is.\n`);
-      repl._refreshLine();
 
     } else if (oldHistoryPath) {
-      // Grab data from the older pre-v3.0 JSON NODE_REPL_HISTORY_FILE format.
+      let threw = false;
-      repl._writeToOutput(
-          '\nConverting old JSON repl history to line-separated history.\n' +
-          `The new repl history file can be found at ${historyPath}.\n`);
-      repl._refreshLine();
-
       try {
         // Pre-v3.0, repl history was stored as JSON.
         // Try and convert it to line separated history.
@@ -153,16 +149,31 @@ function setupHistory(repl, historyPath, oldHistoryPath, ready) {
         // Only attempt to use the history if there was any.
         if (oldReplJSONHistory) repl.history = JSON.parse(oldReplJSONHistory);
 
-        if (!Array.isArray(repl.history)) {
+        if (Array.isArray(repl.history)) {
-          throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
+          repl.history = repl.history.slice(0, repl.historySize);
-                                     typeof repl.history, 'Array');
+        } else {
+          threw = true;
+          _writeToOutput(
+            repl,
+            '\nError: The old history file data has to be an Array.\n' +
+              'REPL session history will not be persisted.\n');
         }
-        repl.history = repl.history.slice(0, repl.historySize);
       } catch (err) {
-        if (err.code !== 'ENOENT') {
+        // Cannot open or parse history file.
-          return ready(
+        // Don't crash, just don't persist history.
-            new errors.Error('ERR_PARSE_HISTORY_DATA', oldHistoryPath));
+        threw = true;
-        }
+        const type = err instanceof SyntaxError ? 'parse' : 'open';
+        _writeToOutput(repl, `\nError: Could not ${type} old history file.\n` +
+                            'REPL session history will not be persisted.\n');
+      }
+      if (!threw) {
+        // Grab data from the older pre-v3.0 JSON NODE_REPL_HISTORY_FILE format.
+        _writeToOutput(
+          repl,
+          '\nConverted old JSON repl history to line-separated history.\n' +
+            `The new repl history file can be found at ${historyPath}.\n`);
+      } else {
+        repl.history = [];
       }
     }
 
@@ -225,12 +236,12 @@ function setupHistory(repl, historyPath, oldHistoryPath, ready) {
 
 function _replHistoryMessage() {
   if (this.history.length === 0) {
-    this._writeToOutput(
+    _writeToOutput(
-        '\nPersistent history support disabled. ' +
+      this,
+      '\nPersistent history support disabled. ' +
         'Set the NODE_REPL_HISTORY environment\nvariable to ' +
         'a valid, user-writable path to enable.\n'
     );
-    this._refreshLine();
   }
   this._historyPrev = Interface.prototype._historyPrev;
   return this._historyPrev();

test/fixtures/old-repl-history-file-faulty.json

@@ -0,0 +1 @@
+undefined

test/fixtures/old-repl-history-file-obj.json

@@ -0,0 +1,4 @@
+{
+  "a": "'=^.^='",
+  "b": "'hello world'"
+}

test/parallel/test-repl-persistent-history.js

@@ -56,13 +56,19 @@ const prompt = '> ';
 const replDisabled = '\nPersistent history support disabled. Set the ' +
                      'NODE_REPL_HISTORY environment\nvariable to a valid, ' +
                      'user-writable path to enable.\n';
-const convertMsg = '\nConverting old JSON repl history to line-separated ' +
+const convertMsg = '\nConverted old JSON repl history to line-separated ' +
                    'history.\nThe new repl history file can be found at ' +
                    path.join(common.tmpDir, '.node_repl_history') + '.\n';
 const homedirErr = '\nError: Could not get the home directory.\n' +
                    'REPL session history will not be persisted.\n';
 const replFailedRead = '\nError: Could not open history file.\n' +
                        'REPL session history will not be persisted.\n';
+const oldHistoryFailedOpen = '\nError: Could not open old history file.\n' +
+                             'REPL session history will not be persisted.\n';
+const oldHistoryFailedParse = '\nError: Could not parse old history file.\n' +
+                              'REPL session history will not be persisted.\n';
+const oldHistoryObj = '\nError: The old history file data has to be an Array' +
+                      '.\nREPL session history will not be persisted.\n';
 const sameHistoryFilePaths = '\nThe old repl history file has the same name ' +
                              'and location as the new one i.e., ' +
                              path.join(common.tmpDir, '.node_repl_history') +
@@ -72,6 +78,10 @@ const fixtures = common.fixturesDir;
 const historyFixturePath = path.join(fixtures, '.node_repl_history');
 const historyPath = path.join(common.tmpDir, '.fixture_copy_repl_history');
 const historyPathFail = path.join(common.tmpDir, '.node_repl\u0000_history');
+const oldHistoryPathObj = path.join(fixtures,
+                                    'old-repl-history-file-obj.json');
+const oldHistoryPathFaulty = path.join(fixtures,
+                                       'old-repl-history-file-faulty.json');
 const oldHistoryPath = path.join(fixtures, 'old-repl-history-file.json');
 const enoentHistoryPath = path.join(fixtures, 'enoent-repl-history-file.json');
 const emptyHistoryPath = path.join(fixtures, '.empty-repl-history-file');
@@ -93,10 +103,19 @@ const tests = [
     expected: [prompt, replDisabled, prompt]
   },
   {
-    env: { NODE_REPL_HISTORY: '',
+    env: { NODE_REPL_HISTORY_FILE: enoentHistoryPath },
-           NODE_REPL_HISTORY_FILE: enoentHistoryPath },
     test: [UP],
-    expected: [prompt, replDisabled, prompt]
+    expected: [prompt, oldHistoryFailedOpen, prompt]
+  },
+  {
+    env: { NODE_REPL_HISTORY_FILE: oldHistoryPathObj },
+    test: [UP],
+    expected: [prompt, oldHistoryObj, prompt]
+  },
+  {
+    env: { NODE_REPL_HISTORY_FILE: oldHistoryPathFaulty },
+    test: [UP],
+    expected: [prompt, oldHistoryFailedParse, prompt]
   },
   {
     env: { NODE_REPL_HISTORY: '',