Skip to content
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.

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

repl: add history support for REPL #434

Closed
wants to merge 1 commit into from

Conversation

evanlucas
Copy link
Contributor

Adds support for saving command history for REPL. When the binary is
run without any arguments or with the -i flag, it will default to
saving the command history to ~/.node_history. Otherwise, it is
disabled by default.

The path to which the history is saved can be changed by setting the
NODE_HISTORY_PATH environment variable. Saving history can be
disabled by setting the NODE_DISABLE_HISTORY environment variable.

See #394

Adds support for saving command history for REPL. When the binary is
run without any arguments or with the `-i` flag, it will default to
saving the command history to ~/.node_history. Otherwise, it is
disabled by default.

The path to which the history is saved can be changed by setting the
`NODE_HISTORY_PATH` environment variable. Saving history can be
disabled by setting the `NODE_DISABLE_HISTORY` environment variable.
@a8m
Copy link

a8m commented Jan 15, 2015

IMO, the persistent history support need to move to the readline module, so that any other cli that's using readline would be able to support too.
e.g:

readline.createInterface({
  input: process.stdin,
  output: process.stdout,
  history: '~/.node_history'
});

Anyway, I'm +1

@chrisdickinson
Copy link
Contributor

Thanks for the PR. While I'm +1 on adding persistent history, I'm -1 on this approach: not all repls should share the same history file. I'd like to see:

  • repl gain the ability to accept history: [array of Strings] as an instantiation option
  • a new, private module lib/_repl.js that handles the history loading, limiting, and persisting
  • src/node.js use lib/_repl.js to provide the REPL.
  • in addition to the environment variables already proposed, a NODE_HISTORY_LENGTH var to limit that file's size.

Ideally, I'd also move the builtin-lib autoloading to lib/_repl.js, but this would be a backwards incompatible change.

Thoughts on this @iojs/tc? Am I missing anything that would make supporting persistent history onerous?

@evanlucas
Copy link
Contributor Author

Would it make sense for readline to get the ability to accept history as well (like @a8m suggested) since that is what is used for history in the first place?

@chrisdickinson
Copy link
Contributor

Yep – if we go this route it should be added to both readline and repl as parameter taking an array of strings representing history entries.


self.useHistory = !!useHistory;
if (self.useHistory) {
self.historyPath = process.env['NODE_HISTORY_PATH'] || historyPath;
Copy link
Member

Choose a reason for hiding this comment

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

This could be dangerous with setuid binaries. (Hopefully no one does that but...)

Copy link
Contributor

Choose a reason for hiding this comment

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

The repl readme contains examples of how to hook up a REPL to a net.Socket for remote inspection – I'm not sure if anyone does this in practice but I feel like it could certainly run afoul of the setuid problem.

A module that's only run when the REPL would be started on the CLI, which only handled things like auto-loading of native modules and persistent history, would sidestep these problems.

@bnoordhuis
Copy link
Member

I agree with @chrisdickinson; the current approach is not bad but it could be generalized more. Moving some of the logic to lib/readline.js is also a good idea.

@rvagg
Copy link
Member

rvagg commented Jan 16, 2015

@chrisdickinson it is used in practice, as evidenced by the popularity of replify:

replify

replify

@btd
Copy link

btd commented Jan 16, 2015

@evanlucas I suggest to take a look how i moved history work in readline, i think we can combine in some way PR. #442

@brendanashworth
Copy link
Contributor

Maybe its just me, but it feels a little bit odd how a regular repl from CLI will have history on by default but a programmatic one wouldn't. It'd make most sense to me to have the same default value for both.

@rvagg rvagg added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 23, 2015
@cjihrig
Copy link
Contributor

cjihrig commented Feb 17, 2015

Closing per #394 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants