-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: persist in plain text, save to home tmp directory by default #2224
Changes from all commits
da336e4
39eb22a
7776ccb
9288067
3acf17d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,9 @@ | |
const Interface = require('readline').Interface; | ||
const REPL = require('repl'); | ||
const path = require('path'); | ||
const fs = require('fs'); | ||
const os = require('os'); | ||
const debug = require('util').debuglog('repl'); | ||
|
||
module.exports = Object.create(REPL); | ||
module.exports.createInternalRepl = createRepl; | ||
|
@@ -17,8 +20,12 @@ function replStart() { | |
return REPL.start.apply(REPL, arguments); | ||
} | ||
|
||
function createRepl(env, cb) { | ||
const opts = { | ||
function createRepl(env, opts, cb) { | ||
if (typeof opts === 'function') { | ||
cb = opts; | ||
opts = null; | ||
} | ||
opts = opts || { | ||
ignoreUndefined: false, | ||
terminal: process.stdout.isTTY, | ||
useGlobal: true | ||
|
@@ -51,15 +58,29 @@ function createRepl(env, cb) { | |
} | ||
|
||
const repl = REPL.start(opts); | ||
if (opts.terminal && env.NODE_REPL_HISTORY_FILE) { | ||
return setupHistory(repl, env.NODE_REPL_HISTORY_FILE, cb); | ||
if (opts.terminal && env.NODE_REPL_HISTORY !== '') { | ||
return setupHistory(repl, env.NODE_REPL_HISTORY, | ||
env.NODE_REPL_HISTORY_FILE, cb); | ||
} | ||
repl._historyPrev = _replHistoryMessage; | ||
cb(null, repl); | ||
} | ||
|
||
function setupHistory(repl, historyPath, ready) { | ||
const fs = require('fs'); | ||
function setupHistory(repl, historyPath, oldHistoryPath, ready) { | ||
if (!historyPath) { | ||
try { | ||
historyPath = path.join(os.homedir(), '.node_repl_history'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In what situation this can throw ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK then I see a need for doc improvement: https://iojs.org/api/os.html#os_os_homedir (unrelated to this PR, of course) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think that it should be mentioned in the doc? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably. |
||
} catch (err) { | ||
repl._writeToOutput('\nError: Could not get the home directory.\n' + | ||
'REPL session history will not be persisted.\n'); | ||
repl._refreshLine(); | ||
|
||
debug(err.stack); | ||
repl._historyPrev = _replHistoryMessage; | ||
return ready(null, repl); | ||
} | ||
} | ||
|
||
var timer = null; | ||
var writing = false; | ||
var pending = false; | ||
|
@@ -86,15 +107,25 @@ function setupHistory(repl, historyPath, ready) { | |
} | ||
|
||
if (data) { | ||
repl.history = data.split(/[\n\r]+/).slice(-repl.historySize); | ||
} else if (oldHistoryPath) { | ||
// Grab data from the older pre-v3.0 JSON NODE_REPL_HISTORY_FILE format. | ||
repl._writeToOutput( | ||
'\nConverting old JSON repl history to line-separated history.\n' + | ||
`The new repl history file can be found at ${historyPath}.\n`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It will only ever do it once, see the |
||
repl._refreshLine(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe move that message to a point after the conversation and point users to unsetting the old env variable so it won't convert again on next run. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #2224 (comment) :) |
||
|
||
try { | ||
repl.history = JSON.parse(data); | ||
repl.history = JSON.parse(fs.readFileSync(oldHistoryPath, 'utf8')); | ||
if (!Array.isArray(repl.history)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can drop this check. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, haha, true. I didn't really know how else to check that it was probably JSON is an reasonable and quick way. |
||
throw new Error('Expected array, got ' + typeof repl.history); | ||
} | ||
repl.history.slice(-repl.historySize); | ||
|
||
repl.history = repl.history.slice(-repl.historySize); | ||
|
||
} catch (err) { | ||
return ready( | ||
new Error(`Could not parse history data in ${historyPath}.`)); | ||
new Error(`Could not parse history data in ${oldHistoryPath}.`)); | ||
} | ||
} | ||
|
||
|
@@ -134,7 +165,7 @@ function setupHistory(repl, historyPath, ready) { | |
return; | ||
} | ||
writing = true; | ||
const historyData = JSON.stringify(repl.history, null, 2); | ||
const historyData = repl.history.join(os.EOL); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if someone wants to share history files across platforms, maybe on a network drive? I think we'd be better off just using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should save using |
||
fs.write(repl._historyHandle, historyData, 0, 'utf8', onwritten); | ||
} | ||
|
||
|
@@ -157,7 +188,7 @@ function _replHistoryMessage() { | |
if (this.history.length === 0) { | ||
this._writeToOutput( | ||
'\nPersistent history support disabled. ' + | ||
'Set the NODE_REPL_HISTORY_FILE environment variable to ' + | ||
'Set the NODE_REPL_HISTORY environment\nvariable to ' + | ||
'a valid, user-writable path to enable.\n' | ||
); | ||
this._refreshLine(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
'you look fabulous today' | ||
'Stay Fresh~' |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
[ | ||
"'=^.^='", | ||
"'hello world'" | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,193 @@ | ||
'use strict'; | ||
|
||
// Flags: --expose-internals | ||
|
||
const common = require('../common'); | ||
const stream = require('stream'); | ||
const REPL = require('internal/repl'); | ||
const assert = require('assert'); | ||
const fs = require('fs'); | ||
const util = require('util'); | ||
const path = require('path'); | ||
const os = require('os'); | ||
|
||
common.refreshTmpDir(); | ||
|
||
// Mock os.homedir() | ||
os.homedir = function() { | ||
return common.tmpDir; | ||
}; | ||
|
||
// Create an input stream specialized for testing an array of actions | ||
class ActionStream extends stream.Stream { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For a test, this is fine, but generally subclassing a stream constructor makes me a liiiiittle nervous. |
||
run(data) { | ||
const _iter = data[Symbol.iterator](); | ||
const self = this; | ||
|
||
function doAction() { | ||
const next = _iter.next(); | ||
if (next.done) { | ||
// Close the repl. Note that it must have a clean prompt to do so. | ||
setImmediate(function() { | ||
self.emit('keypress', '', { ctrl: true, name: 'd' }); | ||
}); | ||
return; | ||
} | ||
const action = next.value; | ||
|
||
if (typeof action === 'object') { | ||
self.emit('keypress', '', action); | ||
} else { | ||
self.emit('data', action + '\n'); | ||
} | ||
setImmediate(doAction); | ||
} | ||
setImmediate(doAction); | ||
} | ||
resume() {} | ||
pause() {} | ||
} | ||
ActionStream.prototype.readable = true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the following code seems to be better than using
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in that case you need to call |
||
|
||
|
||
// Mock keys | ||
const UP = { name: 'up' }; | ||
const ENTER = { name: 'enter' }; | ||
const CLEAR = { ctrl: true, name: 'u' }; | ||
// Common message bits | ||
const prompt = '> '; | ||
const replDisabled = '\nPersistent history support disabled. Set the ' + | ||
'NODE_REPL_HISTORY environment\nvariable to a valid, ' + | ||
'user-writable path to enable.\n'; | ||
const convertMsg = '\nConverting old JSON repl history to line-separated ' + | ||
'history.\nThe new repl history file can be found at ' + | ||
path.join(common.tmpDir, '.node_repl_history') + '.\n'; | ||
const homedirErr = '\nError: Could not get the home directory.\n' + | ||
'REPL session history will not be persisted.\n'; | ||
// File paths | ||
const fixtures = path.join(common.testDir, 'fixtures'); | ||
const historyFixturePath = path.join(fixtures, '.node_repl_history'); | ||
const historyPath = path.join(common.tmpDir, '.fixture_copy_repl_history'); | ||
const oldHistoryPath = path.join(fixtures, 'old-repl-history-file.json'); | ||
|
||
|
||
const tests = [{ | ||
env: { NODE_REPL_HISTORY: '' }, | ||
test: [UP], | ||
expected: [prompt, replDisabled, prompt] | ||
}, | ||
{ | ||
env: { NODE_REPL_HISTORY: '', | ||
NODE_REPL_HISTORY_FILE: oldHistoryPath }, | ||
test: [UP], | ||
expected: [prompt, replDisabled, prompt] | ||
}, | ||
{ | ||
env: { NODE_REPL_HISTORY: historyPath }, | ||
test: [UP, CLEAR], | ||
expected: [prompt, prompt + '\'you look fabulous today\'', prompt] | ||
}, | ||
{ | ||
env: { NODE_REPL_HISTORY: historyPath, | ||
NODE_REPL_HISTORY_FILE: oldHistoryPath }, | ||
test: [UP, CLEAR], | ||
expected: [prompt, prompt + '\'you look fabulous today\'', prompt] | ||
}, | ||
{ | ||
env: { NODE_REPL_HISTORY: historyPath, | ||
NODE_REPL_HISTORY_FILE: '' }, | ||
test: [UP, CLEAR], | ||
expected: [prompt, prompt + '\'you look fabulous today\'', prompt] | ||
}, | ||
{ | ||
env: {}, | ||
test: [UP], | ||
expected: [prompt] | ||
}, | ||
{ | ||
env: { NODE_REPL_HISTORY_FILE: oldHistoryPath }, | ||
test: [UP, CLEAR, '\'42\'', ENTER/*, function(cb) { | ||
// XXX(Fishrock123) Allow the REPL to save to disk. | ||
// There isn't a way to do this programmatically right now. | ||
setTimeout(cb, 50); | ||
}*/], | ||
expected: [prompt, convertMsg, prompt, prompt + '\'=^.^=\'', prompt, '\'', | ||
'4', '2', '\'', '\'42\'\n', prompt, prompt], | ||
after: function ensureHistoryFixture() { | ||
// XXX(Fishrock123) Make sure nothing weird happened to our fixture | ||
// or it's temporary copy. | ||
// Sometimes this test used to erase the fixture and I'm not sure why. | ||
const history = fs.readFileSync(historyFixturePath, 'utf8'); | ||
assert.strictEqual(history, | ||
'\'you look fabulous today\'\n\'Stay Fresh~\'\n'); | ||
const historyCopy = fs.readFileSync(historyPath, 'utf8'); | ||
assert.strictEqual(historyCopy, '\'you look fabulous today\'' + os.EOL + | ||
'\'Stay Fresh~\'' + os.EOL); | ||
} | ||
}, | ||
{ | ||
env: {}, | ||
test: [UP, UP, ENTER], | ||
expected: [prompt, prompt + '\'42\'', prompt + '\'=^.^=\'', '\'=^.^=\'\n', | ||
prompt] | ||
}, | ||
{ // Make sure this is always the last test, since we change os.homedir() | ||
before: function mockHomedirFailure() { | ||
// Mock os.homedir() failure | ||
os.homedir = function() { | ||
throw new Error('os.homedir() failure'); | ||
}; | ||
}, | ||
env: {}, | ||
test: [UP], | ||
expected: [prompt, homedirErr, prompt, replDisabled, prompt] | ||
}]; | ||
|
||
|
||
// Copy our fixture to the tmp directory | ||
fs.createReadStream(historyFixturePath) | ||
.pipe(fs.createWriteStream(historyPath)).on('unpipe', runTest); | ||
|
||
function runTest() { | ||
const opts = tests.shift(); | ||
if (!opts) return; // All done | ||
|
||
const env = opts.env; | ||
const test = opts.test; | ||
const expected = opts.expected; | ||
const after = opts.after; | ||
const before = opts.before; | ||
|
||
if (before) before(); | ||
|
||
REPL.createInternalRepl(env, { | ||
input: new ActionStream(), | ||
output: new stream.Writable({ | ||
write(chunk, _, next) { | ||
const output = chunk.toString(); | ||
|
||
// Ignore escapes and blank lines | ||
if (output.charCodeAt(0) === 27 || /^[\r\n]+$/.test(output)) | ||
return next(); | ||
|
||
assert.strictEqual(output, expected.shift()); | ||
next(); | ||
} | ||
}), | ||
prompt: prompt, | ||
useColors: false, | ||
terminal: true | ||
}, function(err, repl) { | ||
if (err) throw err; | ||
|
||
if (after) repl.on('close', after); | ||
|
||
repl.on('close', function() { | ||
// Ensure everything that we expected was output | ||
assert.strictEqual(expected.length, 0); | ||
setImmediate(runTest); | ||
}); | ||
|
||
repl.inputStream.run(test); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Fishrock123 @rvagg I think the bug is here. We call
setupHistory
with four arguments, but it only takes three.