readline: persistent history + repl changes #596

a8m · 2015-01-24T21:25:54Z

As I commented in #343
I think the persistent history support should be part of the readline module.
After this changes, any cli that using readline would be able to support too.
e.g:

var rl = readline.createInterface({
  input: process.stdin,
  output: process.stdout,
  history: 'path/to/history'
});

The "main" changes is in the readline module and it's tested too.

I also changed a bit the repl module, and add the history support by default.
Maybe it should become with flags(NODE_DISABLE_HISTORY, NODE_HISTORY_PATH) like @evanlucas suggested, but this is out of the scope of this PR.

Thanks.

brendanashworth · 2015-01-24T22:10:15Z

Is there a benefit to loading the current history into memory rather than just appending to the as-is history file? From what I'm seeing, it would allow a programmer to access previous history - at the expense of a breaking change to what they might have previously relied on (i.e., first command ran = first index).

a8m · 2015-01-24T22:43:49Z

Sorry, I didn't get your point.
I try to explain the process:
first, if there's an history support, fetch from the file, and store in-memory.
so, the previous history is available at the start.
then, on each line, store the current history in the file.

brendanashworth · 2015-01-24T23:00:01Z

@a8m its alright. I intended to ask: why load on startup and overwrite the entire file every time a new line is entered, rather than appending each time a new line is entered?

For a big file (lets say 10Mb), and someone enters twenty lines, that'd be ~210Mb of input / output rather than just a few bytes (~8Kb) by appending to the file using fs.appendFileSync(). This would also avoid using lots of RAM by loading the file into memory.

Edit: this would also allow for two readline's to be run at once with the same history file.

a8m · 2015-01-24T23:11:45Z

I agree with you @brendanashworth, it really more sense.
But, because the limit for lines-history(for some reason) it's 30 rows, it's really doesn't matter.
What we can do, it's making it configurable too(make the 30 the default), then append to a file and trim it when it exceeds a particular size.
e.g:

var rl = readline.createInterface({
  //...
  history: 'path/to/history'
});
rl.setHistorySize(100);

what are your thoughts?

brendanashworth · 2015-01-24T23:31:47Z

@a8m I like that idea, 👍 from me on that topic.

On a side note, could you clean up your commit title and description to fit the guidelines?

a8m · 2015-01-25T22:19:39Z

Update:
I've change a bit the previous commit.
Currently, on each line, it's appending to the history file.
Then, when the close function is fired, test if the in-memory history is greater than or equal to _hisotrySize. if so, call writeFileSync and copy the in-memory history to ensure the file is not exceeding the _hisotrySize.(Can you think about more elegant/cheap way to trim the file?).

Also, I've updated the docs and the tests too.
Cheers.

chrisdickinson · 2015-01-26T01:32:20Z

lib/repl.js

@@ -31,6 +31,8 @@ const rl = require('readline');
 const Console = require('console').Console;
 const domain = require('domain');
 const debug = util.debuglog('repl');
+const HISTORY_PATH = path.join(process.env.HOME || process.env.USERPROFILE,
+                               '.iojs_history');


This change should not be made to the REPL module, it should be specific to iojs command line repl.

a8m · 2015-01-26T22:07:24Z

/cc @chrisdickinson

vkurchatkin · 2015-01-27T05:05:54Z

I don't think readline should do any io. The user can just pass history array and retrieve it later

rvagg · 2015-01-27T05:20:21Z

+1 to @vkurchatkin's comment above

a8m · 2015-01-27T08:09:43Z

@vkurchatkin writing to the terminal is also io, but I get your point.
so, if the load and the save operations will move to the user land, it will look something like that:

readline.createInterface({
  //...
  history: loadSync('history')
})
.on('line',  // append to a file)
.on('close', // or save on closing)

If this is acceptable to you guys, I'll make this changes.
Thx.

vkurchatkin · 2015-01-27T09:38:25Z

@a8m it is, but readline doesn't write directly, you provide a stream. This way you can use it with any kind of stream. The same principle applies to history, so that user can store history in memory or database.

piscisaureus · 2015-01-29T16:22:19Z

What @vkurchatkin said.

a8m · 2015-01-29T16:46:21Z

@piscisaureus this PR was updated after @vkurchatkin comments.
Thx

vkurchatkin · 2015-01-29T16:56:45Z

doc/api/readline.markdown

@@ -13,7 +13,8 @@ program to gracefully exit:

    var rl = readline.createInterface({
      input: process.stdin,
-      output: process.stdout
+      output: process.stdout,
+      history: loadHistorySync()


it's not clear from the example what is the result of loadHistorySync(). It's better to use array literal

evanlucas · 2015-01-30T19:32:31Z

lib/readline.js

@@ -24,7 +21,7 @@ exports.createInterface = function(input, output, completer, terminal) {
 };


-function Interface(input, output, completer, terminal) {
+function Interface(input, output, completer, terminal, history) {
  if (!(this instanceof Interface)) {
    return new Interface(input, output, completer, terminal);


history should be passed here as well

Right! (I'm usually using it with option object)
changed!

brendanashworth · 2015-04-13T07:13:52Z

src/node.js

+              (user ? (process.getuid() === 0 ? '/root' : '/home/' + user) : null);
+          }
+          return home || null;
+        })()


style: missing semicolon here

brendanashworth · 2015-04-13T07:15:21Z

Is there an update on this? It hasn't been touched in a while.

a8m · 2015-04-14T09:05:12Z

It's up to you guys.

silverwind · 2015-05-01T12:00:15Z

With 0450ce7 landed, I think this may be obsolete now.

Fishrock123 · 2015-05-01T13:59:27Z

Yep, seems like it.

a8m force-pushed the feature/readline-history branch from f5ee0c9 to dff142f Compare January 25, 2015 21:49

a8m changed the title ~~feat(readline): persistent history + repl changes~~ readline: persistent history + repl changes Jan 25, 2015

chrisdickinson reviewed Jan 26, 2015
View reviewed changes

a8m force-pushed the feature/readline-history branch from dff142f to 62b629b Compare January 26, 2015 22:05

a8m force-pushed the feature/readline-history branch 4 times, most recently from 86fb5d8 to 3755b30 Compare January 28, 2015 06:55

vkurchatkin reviewed Jan 29, 2015
View reviewed changes

a8m force-pushed the feature/readline-history branch from 3755b30 to b577268 Compare January 29, 2015 17:21

evanlucas reviewed Jan 30, 2015
View reviewed changes

a8m force-pushed the feature/readline-history branch from b577268 to 761826c Compare January 30, 2015 19:51

readline: support history + repl changes

b5e62c9

a8m force-pushed the feature/readline-history branch from 761826c to b5e62c9 Compare January 31, 2015 13:03

vkurchatkin mentioned this pull request Jan 31, 2015

repl: save repl session in file #442

Closed

rvagg mentioned this pull request Jan 31, 2015

Pull in sindresorhus/user-home as os.homedir() #682

Closed

brendanashworth added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 3, 2015

mscdex added the readline Issues and PRs related to the built-in readline module. label Mar 22, 2015

brendanashworth reviewed Apr 13, 2015
View reviewed changes

chrisdickinson added the land-on-master label Apr 17, 2015

brendanashworth mentioned this pull request Apr 24, 2015

REPL – Add "magic" mode detection, persistent history support #1513

Closed

Fishrock123 closed this May 1, 2015

This was referenced Jun 22, 2023

[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#506

Open

[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#543

Open

[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#574

Open

leonardoadame mentioned this pull request Jul 7, 2023

[Snyk] Fix for 4 vulnerabilities leonardoadame/node#63

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

readline: persistent history + repl changes #596

readline: persistent history + repl changes #596

a8m commented Jan 24, 2015

brendanashworth commented Jan 24, 2015

a8m commented Jan 24, 2015

brendanashworth commented Jan 24, 2015

a8m commented Jan 24, 2015

brendanashworth commented Jan 24, 2015

a8m commented Jan 25, 2015

chrisdickinson Jan 26, 2015

a8m commented Jan 26, 2015

vkurchatkin commented Jan 27, 2015

rvagg commented Jan 27, 2015

a8m commented Jan 27, 2015

vkurchatkin commented Jan 27, 2015

piscisaureus commented Jan 29, 2015

a8m commented Jan 29, 2015

vkurchatkin Jan 29, 2015

evanlucas Jan 30, 2015

a8m Jan 30, 2015

brendanashworth Apr 13, 2015

brendanashworth commented Apr 13, 2015

a8m commented Apr 14, 2015

silverwind commented May 1, 2015

Fishrock123 commented May 1, 2015

readline: persistent history + repl changes #596

readline: persistent history + repl changes #596

Conversation

a8m commented Jan 24, 2015

brendanashworth commented Jan 24, 2015

a8m commented Jan 24, 2015

brendanashworth commented Jan 24, 2015

a8m commented Jan 24, 2015

brendanashworth commented Jan 24, 2015

a8m commented Jan 25, 2015

chrisdickinson Jan 26, 2015

Choose a reason for hiding this comment

a8m commented Jan 26, 2015

vkurchatkin commented Jan 27, 2015

rvagg commented Jan 27, 2015

a8m commented Jan 27, 2015

vkurchatkin commented Jan 27, 2015

piscisaureus commented Jan 29, 2015

a8m commented Jan 29, 2015

vkurchatkin Jan 29, 2015

Choose a reason for hiding this comment

evanlucas Jan 30, 2015

Choose a reason for hiding this comment

a8m Jan 30, 2015

Choose a reason for hiding this comment

brendanashworth Apr 13, 2015

Choose a reason for hiding this comment

brendanashworth commented Apr 13, 2015

a8m commented Apr 14, 2015

silverwind commented May 1, 2015

Fishrock123 commented May 1, 2015