repl: Enables history for programmatic REPLs. by lance · Pull Request #5789 · nodejs/node

lance · 2016-03-18T19:15:42Z

Pull Request check-list

Please make sure to review and check all of these items:

Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
this change (including linting)?
Is the commit message formatted according to CONTRIBUTING.md?
If this change fixes a bug (or a performance problem), is a regression
test (or a benchmark) included?
Is a documentation update included (if this change modifies
existing APIs, or introduces new ones)?

Affected core subsystem(s)

repl

Description of change

Addresses #5730

The commit moves the setupHistory() function from internal/repl.js
to the public facing REPLServer API. Default behavior has not changed
for either the command line or the programmatic REPL. The
createInternalRepl function used by src/node.js now simply calls
setupHistory() on the REPLServer it creates.

History size for the programmatic REPL may be controlled by providing a
value for the heretofore undocumented historySize in the options for
repl.start().

The commit moves the `setupHistory()` function from `internal/repl.js` to the public facing `REPLServer` API. Default behavior has not changed for either the command line or the programmatic REPL. The `createInternalRepl` function used by `src/node.js` now simply calls `setupHistory()` on the `REPLServer` it creates. History size for the programmatic REPL may be controlled by providing a value for the heretofore undocumented `historySize` in the options for `repl.start()`.

lance · 2016-03-18T19:19:19Z

Not sure if historySize should remain in options. It might make more sense to provide it as a parameter to setupHistory(). I suppose this wouldn't be a breaking change, since it was previously undocumented and therefore perhaps unsupported anyway. It does seem awkward to provide the size as an option, but have to call setupHistory separately.

claudiorodriguez · 2016-03-18T19:57:19Z

A doc update for either the historySize option, and/or setupHistory will definitely be needed if we're making this part of the REPLServer public API. Also, historySize as an option already features in some tests (test-repl and test-repl-options)

lance · 2016-03-18T20:12:32Z

Yeah, I started to update the docs prior to the PR, but figured it would be better to work out these details first.

Fishrock123 · 2016-03-18T20:13:21Z

Note: historySize is handled by readline -- that's the max size of the current in-memory history, and does not effect the persistent saving iirc.

lance · 2016-03-18T20:33:47Z

@Fishrock123 I think it's both. Here the default history size is set on the REPL options. That value is used by REPLServer here to set the current in-memory history for readline. It's also used to limit how much of the history is available on the REPLServer instance itself here. And when the in-memory data is flushed to the filesystem, writes start at position 0 on the history file. If I am reading this correctly, I think the max number of lines you will ever see in the history file is whatever the value of options.historySize is.

Fishrock123 · 2016-03-18T20:50:31Z

Oh right, true. Still, I think the option should be kept where it is. It's just the nature of how things work that it also influences persistence, but that's hardly unwarranted.

Fishrock123 · 2016-03-18T20:51:05Z

cc @chrisdickinson who originally made this functionality (and kept it private)

benjamingr · 2016-03-19T12:51:00Z

Are we sure we want to expose this? This is something external repls can implement in userland with a custom eval function and this increases API surface a little.

victorb · 2016-03-19T18:21:10Z

@benjamingr for me it makes sense to have the ability to use the same functionality no matter if the repl is accessed via the node binary or programmatically in my node program. Otherwise, it makes sense to remove the history from the node cli repl, which would be a loss I think.

benjamingr · 2016-03-19T18:29:32Z

@victorbjelkholm

@benjamingr for me it makes sense to have the ability to use the same functionality no matter if the repl is accessed via the node binary or programmatically in my node program.

I disagree, I think it often makes sense to not expose functionality publicly which lets us break internal APIs, make changes and not commit to a particular API. There are many things and internal modules in core that are not exposed to the public - this might be an exception and exposing it might be worth it but it's certainly not a "expose it or remove it" situation.

Fishrock123 · 2016-03-21T15:03:31Z

 return cmd;
 };

+REPLServer.prototype.setupHistory = function(historyPath, oldHistoryPath, cb) {


At very least, this would have to be refactored so that only the internal one uses oldHistoryPath -- no good in exposing deprecated functionality. :)
Another question this bears is, does the repl try to do this when you create it, or do you have to tell it to? The latter potentially has a race condition I think.

That makes sense. My first jab at this was to keep the existing functionality as similar as possible, in order to get the discussion going without making assumptions or rocking the boat. FWIW, in this PR, oldHistoryPath is optional, so it wouldn't be required by the end user. But yes, it's cleaner to not expose this.
Currently, this is user controlled after the REPL has been created - similar to how it is now in createInternalRepl. I suppose, exposing it to the user as a post-creation action does introduce the race potential.

Fishrock123 · 2016-03-21T15:05:37Z

I'm -0, this isn't really painful to do in useland, but I'm not sure sure I can foresee maintenance problems with it either?

Though, maybe it wait until we use proper file locks on the history? I'm not sure if doing that after the fact could be considered a breaking change.

lance · 2016-03-21T15:05:45Z

@benjamingr @victorbjelkholm I think you both have a valid point. It does make sense to keep the API footprint fairly small and tight. But I agree that providing consistency between the programmatic REPL and the CLI is valid.

benjamingr · 2016-04-10T11:36:48Z

We need to either move forward or close this. I tend towards not merging at this stage but I don't feel strongly about it and if other collaborators feel strongly I don't want to hold it back.

lance · 2016-04-10T12:27:43Z

If there is anything else you all want to see addressed in this PR, let me
know and I'll take care of it.

Lance

On Sunday, April 10, 2016, Benjamin Gruenbaum notifications@github.com
wrote:

We need to either move forward or close this. I tend towards not merging
at this stage but I don't feel strongly about it and if other collaborators
feel strongly I don't want to hold it back.
—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#5789 (comment)

Lance Ball
http://lanceball.com

lance · 2016-04-25T19:38:14Z

Merge or close or modify? Tidying up loose ends.

Fishrock123 · 2016-05-02T17:43:19Z

Let's close it until there is a stronger need for it?

joniba · 2018-10-08T06:53:26Z

I know this is really old, but since this PR wasn't merged, could someone provide an example of how to add this functionality on our own?

joniba · 2018-10-08T07:04:54Z

nevermind: https://medium.com/@tjwebb/a-custom-node-repl-with-history-is-not-as-hard-as-it-looks-3eb2ca7ec0bd

claudiorodriguez added the repl Issues and PRs related to the REPL subsystem. label Mar 18, 2016

Fishrock123 added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 18, 2016

Fishrock123 reviewed Mar 21, 2016
View reviewed changes

estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fbCompare April 26, 2016 05:23

Fishrock123 closed this May 2, 2016

lance mentioned this pull request Feb 2, 2019
repl: add repl.setupHistory for programmatic repl #25895
Closed
4 tasks

Uh oh!

Uh oh!

Conversation

lance commented Mar 18, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

Uh oh!

lance commented Mar 18, 2016

Uh oh!

claudiorodriguez commented Mar 18, 2016

Uh oh!

lance commented Mar 18, 2016

Uh oh!

Fishrock123 commented Mar 18, 2016

Uh oh!

lance commented Mar 18, 2016

Uh oh!

Fishrock123 commented Mar 18, 2016

Uh oh!

Fishrock123 commented Mar 18, 2016

Uh oh!

benjamingr commented Mar 19, 2016

Uh oh!

victorb commented Mar 19, 2016

Uh oh!

benjamingr commented Mar 19, 2016

Uh oh!

Fishrock123Mar 21, 2016

Choose a reason for hiding this comment

Uh oh!

lanceMar 21, 2016

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Mar 21, 2016

Uh oh!

lance commented Mar 21, 2016

Uh oh!

benjamingr commented Apr 10, 2016

Uh oh!

lance commented Apr 10, 2016

Uh oh!

lance commented Apr 25, 2016

Uh oh!

Fishrock123 commented May 2, 2016

Uh oh!

joniba commented Oct 8, 2018

Uh oh!

joniba commented Oct 8, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

joniba commented Oct 8, 2018•
edited
Loading