From a892b4d00c78d3dddc49a0154c3d3f631b5b5c3e Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Thu, 26 Dec 2019 21:00:38 +0100
Subject: [PATCH] repl: improve preview length calculation
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The preview had an off by one error in case colors where deactivated
and did not take fullwidth unicode characters into account when
displaying the preview.

PR-URL: https://github.com/nodejs/node/pull/31112
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
---
 lib/internal/repl/utils.js                    | 56 +++++++++++++------
 test/parallel/test-repl-history-navigation.js |  4 +-
 2 files changed, 41 insertions(+), 19 deletions(-)

diff --git a/lib/internal/repl/utils.js b/lib/internal/repl/utils.js
index 4da23e096c95ed..6eee8d3c273ffa 100644
--- a/lib/internal/repl/utils.js
+++ b/lib/internal/repl/utils.js
@@ -31,26 +31,19 @@ const {
 } = require('readline');
 
 const {
-  commonPrefix
+  commonPrefix,
+  getStringWidth,
 } = require('internal/readline/utils');
 
 const { inspect } = require('util');
 
 const debug = require('internal/util/debuglog').debuglog('repl');
 
-const inspectOptions = {
-  depth: 1,
+const previewOptions = {
   colors: false,
-  compact: true,
-  breakLength: Infinity
-};
-// Specify options that might change the output in a way that it's not a valid
-// stringified object anymore.
-const inspectedOptions = inspect(inspectOptions, {
   depth: 1,
-  colors: false,
   showHidden: false
-});
+};
 
 // If the error is that we've unexpectedly ended the input,
 // then let the user try to recover by adding more input.
@@ -254,7 +247,7 @@ function setupPreview(repl, contextSymbol, bufferSymbol, active) {
         }
         const { result } = preview;
         if (result.value !== undefined) {
-          callback(null, inspect(result.value, inspectOptions));
+          callback(null, inspect(result.value, previewOptions));
         // Ignore EvalErrors, SyntaxErrors and ReferenceErrors. It is not clear
         // where they came from and if they are recoverable or not. Other errors
         // may be inspected.
@@ -264,8 +257,19 @@ function setupPreview(repl, contextSymbol, bufferSymbol, active) {
                      result.className === 'ReferenceError')) {
           callback(null, null);
         } else if (result.objectId) {
+          // The writer options might change and have influence on the inspect
+          // output. The user might change e.g., `showProxy`, `getters` or
+          // `showHidden`. Use `inspect` instead of `JSON.stringify` to keep
+          // `Infinity` and similar intact.
+          const inspectOptions = inspect({
+            ...repl.writer.options,
+            colors: false,
+            depth: 1,
+            compact: true,
+            breakLength: Infinity
+          }, previewOptions);
           session.post('Runtime.callFunctionOn', {
-            functionDeclaration: `(v) => util.inspect(v, ${inspectedOptions})`,
+            functionDeclaration: `(v) => util.inspect(v, ${inspectOptions})`,
             objectId: result.objectId,
             arguments: [result]
           }, (error, preview) => {
@@ -337,15 +341,33 @@ function setupPreview(repl, contextSymbol, bufferSymbol, active) {
       // Limit the output to maximum 250 characters. Otherwise it becomes a)
       // difficult to read and b) non terminal REPLs would visualize the whole
       // output.
-      const maxColumns = MathMin(repl.columns, 250);
-
-      if (inspected.length > maxColumns) {
-        inspected = `${inspected.slice(0, maxColumns - 6)}...`;
+      let maxColumns = MathMin(repl.columns, 250);
+
+      // Support unicode characters of width other than one by checking the
+      // actual width.
+      // TODO(BridgeAR): Add a test case to verify full-width characters work as
+      // expected. Also test that the line break in case of deactivated colors
+      // work as expected.
+      if (inspected.length * 2 >= maxColumns &&
+          getStringWidth(inspected) > maxColumns) {
+        maxColumns -= 4 + (repl.useColors ? 0 : 3);
+        let res = '';
+        for (const char of inspected) {
+          maxColumns -= getStringWidth(char);
+          if (maxColumns < 0)
+            break;
+          res += char;
+        }
+        inspected = `${res}...`;
       }
+
+      // Line breaks are very rare and probably only occur in case of error
+      // messages with line breaks.
       const lineBreakPos = inspected.indexOf('\n');
       if (lineBreakPos !== -1) {
         inspected = `${inspected.slice(0, lineBreakPos)}`;
       }
+
       const result = repl.useColors ?
         `\u001b[90m${inspected}\u001b[39m` :
         `// ${inspected}`;
diff --git a/test/parallel/test-repl-history-navigation.js b/test/parallel/test-repl-history-navigation.js
index 8de2b49b0ea5a6..8dcdf45cf9024d 100644
--- a/test/parallel/test-repl-history-navigation.js
+++ b/test/parallel/test-repl-history-navigation.js
@@ -85,7 +85,7 @@ const tests = [
                  '144, 169, 196, 225, 256, 289, 324, 361, 400, 441, 484, 529,' +
                  ' 576, 625, 676, 729, 784, 841, 900, 961, 1024, 1089, 1156, ' +
                  '1225, 1296, 1369, 1444, 1521, 1600, 1681, 1764, 1849, 1936,' +
-                 ' 2025, 2116, 2209, ...',
+                 ' 2025, 2116, 2209,...',
                `${prompt}{key : {key2 :[] }}`,
                prev && '\n// { key: { key2: [] } }',
                `${prompt}555 + 909`,
@@ -100,7 +100,7 @@ const tests = [
                  '144, 169, 196, 225, 256, 289, 324, 361, 400, 441, 484, 529,' +
                  ' 576, 625, 676, 729, 784, 841, 900, 961, 1024, 1089, 1156, ' +
                  '1225, 1296, 1369, 1444, 1521, 1600, 1681, 1764, 1849, 1936,' +
-                 ' 2025, 2116, 2209, ...',
+                 ' 2025, 2116, 2209,...',
                prompt].filter((e) => typeof e === 'string'),
     clean: true
   },