Skip to content

Commit

Permalink
readline,repl: improve history up/previous
Browse files Browse the repository at this point in the history 
Reaching the history end caused the last entry to be persistent.
That way there's no actualy feedback to the user that the history
end is reached. Instead, visualize the original input line and keep
the history index at the history end in case the user wants to go
back again.

PR-URL: nodejs#31112
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
@BridgeAR @targos
BridgeAR authored and targos committed Apr 25, 2020
1 parent cb0a321 commit fde18ac
Show file tree
Hide file tree
Showing 5 changed files with 90 additions and 28 deletions.
6 changes: 2 additions & 4 deletions lib/readline.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -718,7 +718,7 @@ Interface.prototype._historyNext = function() {
};

Interface.prototype._historyPrev = function() {
if (this.historyIndex < this.history.length) {
if (this.historyIndex < this.history.length && this.history.length) {
const search = this[kSubstringSearch] || '';
let index = this.historyIndex + 1;
while (index < this.history.length &&
Expand All @@ -727,9 +727,7 @@ Interface.prototype._historyPrev = function() {
index++;
}
if (index === this.history.length) {
// TODO(BridgeAR): Change this to:
// this.line = search;
return;
this.line = search;
} else {
this.line = this.history[index];
}
Expand Down
20 changes: 14 additions & 6 deletions test/parallel/test-readline-interface.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -462,12 +462,20 @@ function isWarned(emitter) {
fi.emit('keypress', '.', { name: 'up' }); // 'baz'
assert.strictEqual(rli.historyIndex, 2);
assert.strictEqual(rli.line, 'baz');
fi.emit('keypress', '.', { name: 'up' }); // 'baz'
assert.strictEqual(rli.historyIndex, 2);
assert.strictEqual(rli.line, 'baz');
fi.emit('keypress', '.', { name: 'up' }); // 'baz'
assert.strictEqual(rli.historyIndex, 2);
assert.strictEqual(rli.line, 'baz');
fi.emit('keypress', '.', { name: 'up' }); // 'ba'
assert.strictEqual(rli.historyIndex, 4);
assert.strictEqual(rli.line, 'ba');
fi.emit('keypress', '.', { name: 'up' }); // 'ba'
assert.strictEqual(rli.historyIndex, 4);
assert.strictEqual(rli.line, 'ba');
// Deactivate substring history search and reset history index.
fi.emit('keypress', '.', { name: 'right' }); // 'ba'
assert.strictEqual(rli.historyIndex, -1);
assert.strictEqual(rli.line, 'ba');
// Substring history search activated.
fi.emit('keypress', '.', { name: 'up' }); // 'ba'
assert.strictEqual(rli.historyIndex, 0);
assert.strictEqual(rli.line, 'bat');
rli.close();
}

Expand Down
14 changes: 9 additions & 5 deletions test/parallel/test-repl-history-navigation.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,7 @@ const tests = [
},
{
env: { NODE_REPL_HISTORY: defaultHistoryPath },
checkTotal: true,
test: [UP, UP, UP, UP, UP, DOWN, DOWN, DOWN, DOWN],
test: [UP, UP, UP, UP, UP, DOWN, DOWN, DOWN, DOWN, DOWN],
expected: [prompt,
`${prompt}Array(100).fill(1).map((e, i) => i ** 2)`,
prev && '\n// [ 0, 1, 4, 9, 16, 25, 36, 49, 64, 81, 100, 121, ' +
Expand All @@ -92,6 +91,8 @@ const tests = [
`${prompt}555 + 909`,
prev && '\n// 1464',
`${prompt}let ab = 45`,
prompt,
`${prompt}let ab = 45`,
`${prompt}555 + 909`,
prev && '\n// 1464',
`${prompt}{key : {key2 :[] }}`,
Expand Down Expand Up @@ -138,9 +139,12 @@ const tests = [
// UP - skipping const foo = true
'\x1B[1G', '\x1B[0J',
'> 555 + 909', '\x1B[12G',
// UP, UP, ENTER. UPs at the end of the history have no effect.
'\r\n',
'1464\n',
// UP, UP
// UPs at the end of the history reset the line to the original input.
'\x1B[1G', '\x1B[0J',
'> 55', '\x1B[5G',
// ENTER
'\r\n', '55\n',
'\x1B[1G', '\x1B[0J',
'> ', '\x1B[3G',
'\r\n'
Expand Down
41 changes: 34 additions & 7 deletions test/parallel/test-repl-persistent-history.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const assert = require('assert');
const fs = require('fs');
const path = require('path');
const os = require('os');
const util = require('util');

const tmpdir = require('../common/tmpdir');
tmpdir.refresh();
Expand Down Expand Up @@ -51,6 +52,7 @@ ActionStream.prototype.readable = true;

// Mock keys
const UP = { name: 'up' };
const DOWN = { name: 'down' };
const ENTER = { name: 'enter' };
const CLEAR = { ctrl: true, name: 'u' };

Expand Down Expand Up @@ -90,20 +92,42 @@ const tests = [
},
{
env: {},
test: [UP, '\'42\'', ENTER],
expected: [prompt, '\'', '4', '2', '\'', '\'42\'\n', prompt, prompt],
test: [UP, '21', ENTER, "'42'", ENTER],
expected: [
prompt,
'2', '1', '21\n', prompt, prompt,
"'", '4', '2', "'", "'42'\n", prompt, prompt
],
clean: false
},
{ // Requires the above test case
env: {},
test: [UP, UP, ENTER],
expected: [prompt, `${prompt}'42'`, '\'42\'\n', prompt]
test: [UP, UP, CLEAR, ENTER, DOWN, CLEAR, ENTER, UP, ENTER],
expected: [
prompt,
`${prompt}'42'`,
`${prompt}21`,
prompt,
prompt,
`${prompt}'42'`,
prompt,
prompt,
`${prompt}21`,
'21\n',
prompt,
]
},
{
env: { NODE_REPL_HISTORY: historyPath,
NODE_REPL_HISTORY_SIZE: 1 },
test: [UP, UP, CLEAR],
expected: [prompt, `${prompt}'you look fabulous today'`, prompt]
test: [UP, UP, DOWN, CLEAR],
expected: [
prompt,
`${prompt}'you look fabulous today'`,
prompt,
`${prompt}'you look fabulous today'`,
prompt
]
},
{
env: { NODE_REPL_HISTORY: historyPathFail,
Expand Down Expand Up @@ -169,6 +193,8 @@ function runTest(assertCleaned) {
const opts = tests.shift();
if (!opts) return; // All done

console.log('NEW');

if (assertCleaned) {
try {
assert.strictEqual(fs.readFileSync(defaultHistoryPath, 'utf8'), '');
Expand All @@ -193,6 +219,7 @@ function runTest(assertCleaned) {
output: new stream.Writable({
write(chunk, _, next) {
const output = chunk.toString();
console.log('INPUT', util.inspect(output));

// Ignore escapes and blank lines
if (output.charCodeAt(0) === 27 || /^[\r\n]+$/.test(output))
Expand All @@ -207,7 +234,7 @@ function runTest(assertCleaned) {
next();
}
}),
prompt: prompt,
prompt,
useColors: false,
terminal: true
}, function(err, repl) {
Expand Down
37 changes: 31 additions & 6 deletions test/parallel/test-repl-programmatic-history.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const assert = require('assert');
const fs = require('fs');
const path = require('path');
const os = require('os');
const util = require('util');

const tmpdir = require('../common/tmpdir');
tmpdir.refresh();
Expand Down Expand Up @@ -49,6 +50,7 @@ ActionStream.prototype.readable = true;

// Mock keys
const UP = { name: 'up' };
const DOWN = { name: 'down' };
const ENTER = { name: 'enter' };
const CLEAR = { ctrl: true, name: 'u' };

Expand Down Expand Up @@ -88,20 +90,40 @@ const tests = [
},
{
env: {},
test: [UP, '\'42\'', ENTER],
expected: [prompt, '\'', '4', '2', '\'', '\'42\'\n', prompt, prompt],
test: [UP, '21', ENTER, "'42'", ENTER],
expected: [
prompt,
// TODO(BridgeAR): The line is refreshed too many times. The double prompt
// is redundant and can be optimized away.
'2', '1', '21\n', prompt, prompt,
"'", '4', '2', "'", "'42'\n", prompt, prompt
],
clean: false
},
{ // Requires the above test case
env: {},
test: [UP, UP, ENTER],
expected: [prompt, `${prompt}'42'`, '\'42\'\n', prompt]
test: [UP, UP, UP, DOWN, ENTER],
expected: [
prompt,
`${prompt}'42'`,
`${prompt}21`,
prompt,
`${prompt}21`,
'21\n',
prompt
]
},
{
env: { NODE_REPL_HISTORY: historyPath,
NODE_REPL_HISTORY_SIZE: 1 },
test: [UP, UP, CLEAR],
expected: [prompt, `${prompt}'you look fabulous today'`, prompt]
test: [UP, UP, DOWN, CLEAR],
expected: [
prompt,
`${prompt}'you look fabulous today'`,
prompt,
`${prompt}'you look fabulous today'`,
prompt
]
},
{
env: { NODE_REPL_HISTORY: historyPathFail,
Expand Down Expand Up @@ -167,6 +189,8 @@ function runTest(assertCleaned) {
const opts = tests.shift();
if (!opts) return; // All done

console.log('NEW');

if (assertCleaned) {
try {
assert.strictEqual(fs.readFileSync(defaultHistoryPath, 'utf8'), '');
Expand All @@ -192,6 +216,7 @@ function runTest(assertCleaned) {
output: new stream.Writable({
write(chunk, _, next) {
const output = chunk.toString();
console.log('INPUT', util.inspect(output));

// Ignore escapes and blank lines
if (output.charCodeAt(0) === 27 || /^[\r\n]+$/.test(output))
Expand Down

0 comments on commit fde18ac

Please sign in to comment.