Skip to content

repl: runtime deprecate NODE_REPL_HISTORY_FILE #16505

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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

repl: runtime deprecate NODE_REPL_HISTORY_FILE #16505

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ad7c486
repl: runtime deprecate NODE_REPL_HISTORY_FILE
maclover7 Oct 25, 2017
a133000
[squash] move dep to separate function, add tests
maclover7 Oct 26, 2017
4f3c85a
fix lint error
maclover7 Nov 30, 2017
d17c56a
fix nits
maclover7 Dec 4, 2017
da8d65b
remove extra space
maclover7 Dec 4, 2017
9a8cf0d
squash
maclover7 Jan 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion doc/api/deprecations.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ instead.
<a id="DEP0041"></a>
### DEP0041: NODE\_REPL\_HISTORY\_FILE environment variable

Type: Documentation-only
Type: Runtime

The `NODE_REPL_HISTORY_FILE` environment variable has been deprecated.

Expand Down
9 changes: 9 additions & 0 deletions lib/internal/repl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,13 @@ const debug = util.debuglog('repl');
module.exports = Object.create(REPL);
module.exports.createInternalRepl = createRepl;

const { deprecate } = require('internal/util');
const _printHistoryDeprecation = deprecate(
() => {},
'NODE_REPL_HISTORY_FILE is deprecated. Use NODE_REPL_HISTORY instead.',
'DEP0041'
);

// XXX(chrisdickinson): The 15ms debounce value is somewhat arbitrary.
// The debounce is to guard against code pasted into the REPL.
const kDebounceHistoryMS = 15;
Expand Down Expand Up @@ -167,6 +174,8 @@ function setupHistory(repl, historyPath, oldHistoryPath, ready) {
'REPL session history will not be persisted.\n');
}
if (!threw) {
_printHistoryDeprecation();

// Grab data from the older pre-v3.0 JSON NODE_REPL_HISTORY_FILE format.
_writeToOutput(
repl,
Expand Down
20 changes: 20 additions & 0 deletions test/parallel/test-repl-persistent-history.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,11 +122,13 @@ const tests = [
expected: [prompt, replDisabled, prompt]
},
{
deprecated: true,
env: { NODE_REPL_HISTORY_FILE: emptyHistoryPath },
test: [UP],
expected: [prompt, convertMsg, prompt]
},
{
deprecated: true,
env: { NODE_REPL_HISTORY_FILE: defaultHistoryPath },
test: [UP],
expected: [prompt, sameHistoryFilePaths, prompt]
Expand Down Expand Up @@ -154,6 +156,7 @@ const tests = [
expected: [prompt]
},
{
deprecated: true,
env: { NODE_REPL_HISTORY_FILE: oldHistoryPath },
test: [UP, CLEAR, '\'42\'', ENTER],
expected: [prompt, convertMsg, prompt, `${prompt}'=^.^='`, prompt, '\'',
Expand All @@ -173,6 +176,7 @@ const tests = [
expected: [prompt, `${prompt}'you look fabulous today'`, prompt]
},
{
deprecated: true,
env: { NODE_REPL_HISTORY_FILE: oldHistoryPath,
NODE_REPL_HISTORY_SIZE: 1 },
test: [UP, UP, UP, CLEAR],
Expand Down Expand Up @@ -258,9 +262,25 @@ function runTest(assertCleaned) {
const expected = opts.expected;
const clean = opts.clean;
const before = opts.before;
const deprecated = !!opts.deprecated;

const depMsg = /NODE_REPL_HISTORY_FILE is deprecated\. Use NODE_REPL_HISTORY instead\./;

if (before) before();

// TODO: Fix this. There is a bug where env variables/state are shared
// between tests. That is why the `if deprecated` is needed, or else tests
// will be failing all over the place for no reason. It would better to not
// have the guard, because otherwise what if tests that don't have deprecated
// set being to have deprecation warnings printed? :(
if (deprecated) {
common.hijackStderr(function(data) {
if (data) {
assert.ok(deprecated && depMsg.test(data));
}
});
}
Copy link
Member

Choose a reason for hiding this comment

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

To me it looks like the hijack is called multiple times and will therefore also trigger for tests that have nothing to do with the current test.
So I think the hijack should be deactivated as soon as it is not required anymore.

In that case the inner deprecated check should also be obsolete.


REPL.createInternalRepl(env, {
input: new ActionStream(),
output: new stream.Writable({
Expand Down