repl: persist in plain text, save to home tmp directory by default #2224

Fishrock123 · 2015-07-22T16:51:06Z

I know @chrisdickinson said it would have been better to have the repl save in plain text, and that doing so would require a semver-major change.

I'd like to get it into 3.0 if possible, and I'm willing to do whatever if it looks good to people.

This also effectively enables it by default, by using os.homedir() to get an appropriate location (I think?). Perhaps I should be handling more errors if it can't exist?

The first commit is the important one.

R= @chrisdickinson or @rvagg since he's doing 3.0?

targos · 2015-07-22T17:17:11Z

lib/internal/repl.js

I think we can drop this check.
With the new condition at lines 99-100, we are sure it is an array.

Oh, haha, true.

I didn't really know how else to check that it was probably JSON is an reasonable and quick way.

targos · 2015-07-22T17:56:14Z

lib/internal/repl.js

I know you didn't touch it but this line has no effect. It should be repl.history = repl.history.slice(-repl.historySize);

btw it feels wrong to slice it here. Why isn't it done before saving the file in flushHistory ?

Good catch.

flushHistory() could probably use some fixes/improvements. It always appends to the file no matter what the current file size is.

It shouldn't be permanently appending, since we're using this variant of fs.write, with position set to 0. The existing repl logic should be handling the trimming of history.

(but yes, we should assign repl.history = repl.history.slice(...) here.)

Fishrock123 · 2015-07-23T00:17:39Z

Updated based off on this length discussion with @chrisdickinson on IRC: http://logs.libuv.org/io.js/2015-07-22#18:19:50.744

We now prefer a new NODE_REPL_HISTORY env variable, and only try to parse JSON the first time if NODE_REPL_HISTORY_FILE is specified.

Still needs some extra log messages about all of this, especially if people are still afterwards specifying a NODE_REPL_HISTORY_FILE.

rvagg · 2015-07-23T00:59:05Z

lib/internal/repl.js

It is possible for os.homedir() to throw, it would be very uncommon but there are some scenarios beyond ENOMEM, ENOBUFS, ENOENT, EMFILE etc. that might be useful to report rather than just crash. I wonder if we should try/catch this and early return ready()?

Oh, whoops, I had forgotten about that, thanks.

It'd probably depend on the error what we may want to do; there a difference between say ENOMEM and ENOENT. :P

Hmm, I'm not really so sure what I meant with this last night! I guess we should just log the error stack and then call ready?

mscdex · 2015-07-24T02:54:46Z

I haven't been following the repl history discussion, but what about keeping JSON and just using exclusive locks on the history file?

Fishrock123 · 2015-07-24T04:28:28Z

I haven't been following the repl history discussion, but what about keeping JSON and just using exclusive locks on the history file?

Doing thing atomically is another must-do sometime soon. (But I currently don't know how to do that at all.)

This however is more about moving off of json for thoughts of of general stability and API going forward, especially as we near a converged release. It just seems like a better option future-wise, so I felt it was best not to delay it; might as well do it now rather than when people might actually rely on it being json for one reason or other.

silverwind · 2015-07-27T10:13:06Z

lib/internal/repl.js

What if someone wants to share history files across platforms, maybe on a network drive? I think we'd be better off just using \n here.

We should save using os.EOL, and split liberally on either line ending.

Fishrock123 · 2015-07-29T06:58:10Z

I think I've addressed basically everything. We don't really have tests for this, right?

I just need to improve the "converting" info message.

Also it now only tries to load the old history if there isn't any new history.

targos · 2015-07-29T09:20:17Z

lib/internal/repl.js

I think you should split with /[\r\n]+/ for Windows compatibility.

or os.EOL?

@thefourtheye see above comment from @silverwind .
The idea is to be able to share the history file between different platforms.

Oh yeah, thanks for pointing out :-)

/\n|\r/ will result in empty lines when reading \r\n files. better use /\r?\n/ or even /\r?\n|\n?\r/ if you want to be totally safe.

/\r\n|\r|\n/ would be more explicit, right?

Wouldn't catch a \n\r case, if such a platform exists, so you'd need /\r\n|\n\r|\r|\n/ but I really have no preference here.

Based on os.EOL, /\r?\n/ would suffice.

FWIW the regex I suggested has an interesting side-effect: empty lines will be ignored.

@targos oh indeed. That might be a nice 'feature' ;)

Fishrock123 · 2015-07-29T18:29:02Z

Updated with everything, PTAL. + cc @chrisdickinson & @rvagg again

Fishrock123 · 2015-07-29T18:29:46Z

lib/internal/repl.js

Will fix this line when squashing

chrisdickinson · 2015-07-29T19:28:05Z

Tiny nit re: message, but LGTM.

Fishrock123 · 2015-07-30T02:00:29Z

Now includes some doc updates. Should I mention that NODE_REPL_HISTORY_FILE is deprecated, or not at all?

... I'll try to add that bit in later tonight.

PR-URL: #2224 Reviewed-By: Michaël Zasso <mic.besace@gmail.com> Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com> Reviewed-By: Roman Reiss <me@silverwind.io>

targos · 2015-08-05T11:40:26Z

landed in d200932...ed6c249

arty-name · 2015-08-07T11:15:12Z

would be nice to support XDG Base Directory Specification for the default path

akiva · 2015-10-22T18:37:56Z

+1 @arty-name on the XDG spec standard

Fishrock123 added repl Issues and PRs related to the REPL subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Jul 22, 2015

Fishrock123 added this to the 3.0.0 milestone Jul 22, 2015

targos reviewed Jul 22, 2015
View reviewed changes

Fishrock123 force-pushed the repl-history-nojson branch from a440565 to 8664f5e Compare July 22, 2015 17:36

targos reviewed Jul 22, 2015
View reviewed changes

indutny force-pushed the master branch from bffb204 to eb35968 Compare July 22, 2015 21:21

rvagg reviewed Jul 23, 2015
View reviewed changes

silverwind reviewed Jul 27, 2015
View reviewed changes

targos reviewed Jul 29, 2015
View reviewed changes

Fishrock123 reviewed Jul 29, 2015
View reviewed changes

lib/internal/repl.js Outdated

Copy link

Contributor Author

Fishrock123 Jul 29, 2015

Choose a reason for hiding this comment

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

Will fix this line when squashing

rvagg mentioned this pull request Aug 4, 2015

Release proposal: v3.0.0 #2299

Merged

targos closed this Aug 5, 2015

BridgeAR mentioned this pull request Dec 2, 2018

repl: handling multiline history needs some case guards #24231

Closed

jasonkarns mentioned this pull request Mar 2, 2019

repl: add repl.setupHistory for programmatic repl #25895

Closed

4 tasks

Uh oh!

repl: persist in plain text, save to home tmp directory by default #2224

repl: persist in plain text, save to home tmp directory by default #2224

Uh oh!

Conversation

Fishrock123 commented Jul 22, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Jul 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Jul 24, 2015

Uh oh!

Fishrock123 commented Jul 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Jul 29, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Jul 29, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisdickinson commented Jul 29, 2015

Uh oh!

Fishrock123 commented Jul 30, 2015

Uh oh!

targos commented Aug 5, 2015

Uh oh!

arty-name commented Aug 7, 2015

Uh oh!

akiva commented Oct 22, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!