Skip to content

repl: extract and standardize history from both repl and interface #58225

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 28, 2025
Merged

repl: extract and standardize history from both repl and interface #58225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions doc/api/repl.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -645,14 +645,35 @@ buffered but not yet executed. This method is primarily intended to be
called from within the action function for commands registered using the
`replServer.defineCommand()` method.

### `replServer.setupHistory(historyPath, callback)`
### `replServer.setupHistory(historyConfig, callback)`

<!-- YAML
added: v11.10.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/58225
description: Updated the `historyConfig` parameter to accept an object
with `filePath`, `size`, `removeHistoryDuplicates` and
`onHistoryFileLoaded` properties.
-->

* `historyPath` {string} the path to the history file
* `historyConfig` {Object|string} the path to the history file
If it is a string, it is the path to the history file.
If it is an object, it can have the following properties:
* `filePath` {string} the path to the history file
* `size` {number} Maximum number of history lines retained. To disable
the history set this value to `0`. This option makes sense only if
`terminal` is set to `true` by the user or by an internal `output` check,
otherwise the history caching mechanism is not initialized at all.
**Default:** `30`.
* `removeHistoryDuplicates` {boolean} If `true`, when a new input line added
to the history list duplicates an older one, this removes the older line
from the list. **Default:** `false`.
* `onHistoryFileLoaded` {Function} called when history writes are ready or upon error
* `err` {Error}
* `repl` {repl.REPLServer}
* `callback` {Function} called when history writes are ready or upon error
(Optional if provided as `onHistoryFileLoaded` in `historyConfig`)
* `err` {Error}
* `repl` {repl.REPLServer}

Expand Down
2 changes: 1 addition & 1 deletion lib/internal/main/repl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ if (process.env.NODE_REPL_EXTERNAL_MODULE) {
throw err;
}
repl.on('exit', () => {
if (repl._flushing) {
if (repl.historyManager.isFlushing) {
return repl.once('flushHistory', () => {
process.exit();
});
Expand Down
192 changes: 57 additions & 135 deletions lib/internal/readline/interface.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,12 @@
const {
ArrayFrom,
ArrayPrototypeFilter,
ArrayPrototypeIndexOf,
ArrayPrototypeJoin,
ArrayPrototypeMap,
ArrayPrototypePop,
ArrayPrototypePush,
ArrayPrototypeReverse,
ArrayPrototypeShift,
ArrayPrototypeSplice,
ArrayPrototypeUnshift,
DateNow,
FunctionPrototypeCall,
Expand All @@ -19,6 +17,7 @@ const {
MathMax,
MathMaxApply,
NumberIsFinite,
ObjectDefineProperty,
ObjectSetPrototypeOf,
RegExpPrototypeExec,
SafeStringIterator,
Expand All @@ -30,7 +29,6 @@ const {
StringPrototypeSlice,
StringPrototypeSplit,
StringPrototypeStartsWith,
StringPrototypeTrim,
Symbol,
SymbolAsyncIterator,
SymbolDispose,
Expand All @@ -46,8 +44,6 @@ const {

const {
validateAbortSignal,
validateArray,
validateNumber,
validateString,
validateUint32,
} = require('internal/validators');
Expand All @@ -67,7 +63,6 @@ const {
charLengthLeft,
commonPrefix,
kSubstringSearch,
reverseString,
} = require('internal/readline/utils');
let emitKeypressEvents;
let kFirstEventParam;
Expand All @@ -78,8 +73,8 @@ const {
} = require('internal/readline/callbacks');

const { StringDecoder } = require('string_decoder');
const { ReplHistory } = require('internal/repl/history');

const kHistorySize = 30;
const kMaxUndoRedoStackSize = 2048;
const kMincrlfDelay = 100;
/**
Expand Down Expand Up @@ -153,7 +148,6 @@ const kWriteToOutput = Symbol('_writeToOutput');
const kYank = Symbol('_yank');
const kYanking = Symbol('_yanking');
const kYankPop = Symbol('_yankPop');
const kNormalizeHistoryLineEndings = Symbol('_normalizeHistoryLineEndings');
const kSavePreviousState = Symbol('_savePreviousState');
const kRestorePreviousState = Symbol('_restorePreviousState');
const kPreviousLine = Symbol('_previousLine');
Expand All @@ -175,9 +169,6 @@ function InterfaceConstructor(input, output, completer, terminal) {

FunctionPrototypeCall(EventEmitter, this);

let history;
let historySize;
let removeHistoryDuplicates = false;
let crlfDelay;
let prompt = '> ';
let signal;
Expand All @@ -187,14 +178,17 @@ function InterfaceConstructor(input, output, completer, terminal) {
output = input.output;
completer = input.completer;
terminal = input.terminal;
history = input.history;
historySize = input.historySize;
signal = input.signal;

// It is possible to configure the history through the input object
const historySize = input.historySize;
const history = input.history;
const removeHistoryDuplicates = input.removeHistoryDuplicates;

if (input.tabSize !== undefined) {
validateUint32(input.tabSize, 'tabSize', true);
this.tabSize = input.tabSize;
}
removeHistoryDuplicates = input.removeHistoryDuplicates;
if (input.prompt !== undefined) {
prompt = input.prompt;
}
Expand All @@ -215,24 +209,18 @@ function InterfaceConstructor(input, output, completer, terminal) {

crlfDelay = input.crlfDelay;
input = input.input;
}

if (completer !== undefined && typeof completer !== 'function') {
throw new ERR_INVALID_ARG_VALUE('completer', completer);
input.size = historySize;
input.history = history;
input.removeHistoryDuplicates = removeHistoryDuplicates;
}

if (history === undefined) {
history = [];
} else {
validateArray(history, 'history');
}
this.setupHistoryManager(input);

if (historySize === undefined) {
historySize = kHistorySize;
if (completer !== undefined && typeof completer !== 'function') {
throw new ERR_INVALID_ARG_VALUE('completer', completer);
}

validateNumber(historySize, 'historySize', 0);

// Backwards compat; check the isTTY prop of the output stream
// when `terminal` was not specified
if (terminal === undefined && !(output === null || output === undefined)) {
Expand All @@ -248,8 +236,6 @@ function InterfaceConstructor(input, output, completer, terminal) {
this.input = input;
this[kUndoStack] = [];
this[kRedoStack] = [];
this.history = history;
this.historySize = historySize;
this[kPreviousCursorCols] = -1;

// The kill ring is a global list of blocks of text that were previously
Expand All @@ -260,7 +246,6 @@ function InterfaceConstructor(input, output, completer, terminal) {
this[kKillRing] = [];
this[kKillRingCursor] = 0;

this.removeHistoryDuplicates = !!removeHistoryDuplicates;
this.crlfDelay = crlfDelay ?
MathMax(kMincrlfDelay, crlfDelay) :
kMincrlfDelay;
Expand All @@ -270,7 +255,6 @@ function InterfaceConstructor(input, output, completer, terminal) {

this.terminal = !!terminal;


function onerror(err) {
self.emit('error', err);
}
Expand Down Expand Up @@ -349,8 +333,6 @@ function InterfaceConstructor(input, output, completer, terminal) {
// Cursor position on the line.
this.cursor = 0;

this.historyIndex = -1;

if (output !== null && output !== undefined)
output.on('resize', onresize);

Expand Down Expand Up @@ -403,6 +385,36 @@ class Interface extends InterfaceConstructor {
return this[kPrompt];
}

setupHistoryManager(options) {
this.historyManager = new ReplHistory(this, options);

if (options.onHistoryFileLoaded) {
this.historyManager.initialize(options.onHistoryFileLoaded);
}
Comment on lines +391 to +393
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it correct that you initialize the history manager only if an onHistoryFileLoaded is provided? shouldn't initialize always be run and with a no-op callback if there is no onHistoryFileLoaded ? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's not how the current implementation works. The point of this PR is just to extract existing history functionality into a separate module because we realized that both readline and repl are too dependent on it; anything that can be improved, can be done in a separate PR 😄 .
When I tried doing what you suggested, countless things broke because that callback is only supposed to run in certain particular cases

Copy link
Member

@dario-piotrowicz dario-piotrowicz May 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of this PR is just to extract existing history functionality into a separate module because we realized that both readline and repl are too dependent on it; anything that can be improved, can be done in a separate PR 😄 .

Agreed 🙂👍

When I tried doing what you suggested, countless things broke because that callback is only supposed to run in certain particular cases

Thanks for giving that a try 🙂👍

that's not how the current implementation works.

I actually gave this a quick try and it looks like your implemented behavior is diverging from what node currently does, currently if the callback is not provided an error is thrown, with your changes this doesn't happen, see:

Screenshot at 2025-05-26 11-49-38

(my-node points to my local build of node, which I built on your branch)

would it be worth it to validate the callback and keep the current behavior? 🤔

PS: node is failing here:

node/lib/internal/repl/history.js

Line 121 in 19e0d47

ready(null, repl);

so it's not 100% clear to me if this erroring is intentional or not... at the very least this makes me thing that an assumption is being made that the callback is always defined 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fascinating, if I always call the callback (even with an empty one, when not provided) I get the exact same error (complaining about ready) but just in the test cases. Tbh, I like that it does not crash and it is still usable when not provided 😃


ObjectDefineProperty(this, 'history', {
__proto__: null, configurable: true, enumerable: true,
get() { return this.historyManager.history; },
set(newHistory) { return this.historyManager.history = newHistory; },
});

ObjectDefineProperty(this, 'historyIndex', {
__proto__: null, configurable: true, enumerable: true,
get() { return this.historyManager.index; },
set(historyIndex) { return this.historyManager.index = historyIndex; },
});

ObjectDefineProperty(this, 'historySize', {
__proto__: null, configurable: true, enumerable: true,
get() { return this.historyManager.size; },
});

ObjectDefineProperty(this, 'isFlushing', {
__proto__: null, configurable: true, enumerable: true,
get() { return this.historyManager.isFlushing; },
});
}

[kSetRawMode](mode) {
const wasInRawMode = this.input.isRaw;

Expand Down Expand Up @@ -478,70 +490,8 @@ class Interface extends InterfaceConstructor {
}
}

// Convert newlines to a consistent format for history storage
[kNormalizeHistoryLineEndings](line, from, to, reverse = true) {
// Multiline history entries are saved reversed
// History is structured with the newest entries at the top
// and the oldest at the bottom. Multiline histories, however, only occupy
// one line in the history file. When loading multiline history with
// an old node binary, the history will be saved in the old format.
// This is why we need to reverse the multilines.
// Reversing the multilines is necessary when adding / editing and displaying them
if (reverse) {
// First reverse the lines for proper order, then convert separators
return reverseString(line, from, to);
}
// For normal cases (saving to history or non-multiline entries)
return StringPrototypeReplaceAll(line, from, to);
}

[kAddHistory]() {
if (this.line.length === 0) return '';

// If the history is disabled then return the line
if (this.historySize === 0) return this.line;

// If the trimmed line is empty then return the line
if (StringPrototypeTrim(this.line).length === 0) return this.line;

// This is necessary because each line would be saved in the history while creating
// A new multiline, and we don't want that.
if (this[kIsMultiline] && this.historyIndex === -1) {
ArrayPrototypeShift(this.history);
} else if (this[kLastCommandErrored]) {
// If the last command errored and we are trying to edit the history to fix it
// Remove the broken one from the history
ArrayPrototypeShift(this.history);
}

const normalizedLine = this[kNormalizeHistoryLineEndings](this.line, '\n', '\r', true);

if (this.history.length === 0 || this.history[0] !== normalizedLine) {
if (this.removeHistoryDuplicates) {
// Remove older history line if identical to new one
const dupIndex = ArrayPrototypeIndexOf(this.history, this.line);
if (dupIndex !== -1) ArrayPrototypeSplice(this.history, dupIndex, 1);
}

// Add the new line to the history
ArrayPrototypeUnshift(this.history, normalizedLine);

// Only store so many
if (this.history.length > this.historySize)
ArrayPrototypePop(this.history);
}

this.historyIndex = -1;

// The listener could change the history object, possibly
// to remove the last added entry if it is sensitive and should
// not be persisted in the history, like a password
const line = this[kIsMultiline] ? reverseString(this.history[0]) : this.history[0];

// Emit history event to notify listeners of update
this.emit('history', this.history);

return line;
return this.historyManager.addHistory(this[kIsMultiline], this[kLastCommandErrored]);
}

[kRefreshLine]() {
Expand Down Expand Up @@ -1184,26 +1134,12 @@ class Interface extends InterfaceConstructor {
// <ctrl> + N. Only show this after two/three UPs or DOWNs, not on the first
// one.
[kHistoryNext]() {
if (this.historyIndex >= 0) {
this[kBeforeEdit](this.line, this.cursor);
const search = this[kSubstringSearch] || '';
let index = this.historyIndex - 1;
while (
index >= 0 &&
(!StringPrototypeStartsWith(this.history[index], search) ||
this.line === this.history[index])
) {
index--;
}
if (index === -1) {
this[kSetLine](search);
} else {
this[kSetLine](this[kNormalizeHistoryLineEndings](this.history[index], '\r', '\n'));
}
this.historyIndex = index;
this.cursor = this.line.length; // Set cursor to end of line.
this[kRefreshLine]();
}
if (!this.historyManager.canNavigateToNext()) { return; }

this[kBeforeEdit](this.line, this.cursor);
this[kSetLine](this.historyManager.navigateToNext(this[kSubstringSearch]));
this.cursor = this.line.length; // Set cursor to end of line.
this[kRefreshLine]();
}

[kMoveUpOrHistoryPrev]() {
Expand All @@ -1218,26 +1154,12 @@ class Interface extends InterfaceConstructor {
}

[kHistoryPrev]() {
if (this.historyIndex < this.history.length && this.history.length) {
this[kBeforeEdit](this.line, this.cursor);
const search = this[kSubstringSearch] || '';
let index = this.historyIndex + 1;
while (
index < this.history.length &&
(!StringPrototypeStartsWith(this.history[index], search) ||
this.line === this.history[index])
) {
index++;
}
if (index === this.history.length) {
this[kSetLine](search);
} else {
this[kSetLine](this[kNormalizeHistoryLineEndings](this.history[index], '\r', '\n'));
}
this.historyIndex = index;
this.cursor = this.line.length; // Set cursor to end of line.
this[kRefreshLine]();
}
if (!this.historyManager.canNavigateToPrevious()) { return; }

this[kBeforeEdit](this.line, this.cursor);
this[kSetLine](this.historyManager.navigateToPrevious(this[kSubstringSearch]));
this.cursor = this.line.length; // Set cursor to end of line.
this[kRefreshLine]();
}

// Returns the last character's display position of the given string
Expand Down
Loading
Loading