From 6cc2713cfe9b6f954cf57c9f02f348de50344d00 Mon Sep 17 00:00:00 2001 From: Lance Ball Date: Wed, 2 Mar 2016 16:45:16 -0500 Subject: [PATCH 1/6] repl: Assignment of _ allowed with warning This commit addresses https://github.com/nodejs/node/issues/5431 by changing the way that the repl handles assignment to the global _ variable. Prior to this commit, node sets the result of the last expression evaluated in the repl to `_`. This causes problems for users of underscore, lodash and other packages where it is common to assign `_` to the package, e.g. `_ = require('lodash');`. Changes in this commit now result in the following behavior. - If unassigned on the repl, `_` continues to refer to the last evaluated expression. - If assigned, the default behavior of assigning `_` to the last evaluated expression is disabled, and `_` now references whatever value was explicitly set. A warning is issued on the repl - 'expression assignment to _ now disabled'. - If `_` is assigned multiple times, the warning is only displayed once. - When `.clear` is executed in the repl, `_` continues to refer to its most recent value, whatever that is (this is per existing behavior). If `_` had been explicitly set prior to `.clear` it will not change again with the evaluation of the next expression. --- lib/repl.js | 22 ++++++++++-- test/parallel/test-repl-underscore.js | 49 +++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-repl-underscore.js diff --git a/lib/repl.js b/lib/repl.js index ab9f1bf56dce1e..6da70eba0d5751 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -205,6 +205,8 @@ function REPLServer(prompt, self.useGlobal = !!useGlobal; self.ignoreUndefined = !!ignoreUndefined; self.replMode = replMode || exports.REPL_MODE_SLOPPY; + self.setLast = true; + self.last = undefined; self._inTemplateLiteral = false; @@ -471,7 +473,7 @@ function REPLServer(prompt, // the second argument to this function is there, print it. arguments.length === 2 && (!self.ignoreUndefined || ret !== undefined)) { - self.context._ = ret; + if (self.setLast) self.last = ret; self.outputStream.write(self.writer(ret) + '\n'); } @@ -545,6 +547,8 @@ REPLServer.prototype.createContext = function() { context.module = module; context.require = require; + const repl = this; + this.lines = []; this.lines.level = []; @@ -554,7 +558,7 @@ REPLServer.prototype.createContext = function() { Object.defineProperty(context, name, { get: function() { var lib = require(name); - context._ = context[name] = lib; + repl.last = context[name] = lib; return lib; }, // allow the creation of other globals with this name @@ -566,6 +570,20 @@ REPLServer.prototype.createContext = function() { }); }); + Object.defineProperty(context, '_', { + configurable: true, + get: function() { + return repl.last; + }, + set: function(value) { + repl.last = value; + if (repl.setLast) { + repl.setLast = false; + repl.outputStream.write('expression assignment to _ now disabled\n'); + } + } + }); + return context; }; diff --git a/test/parallel/test-repl-underscore.js b/test/parallel/test-repl-underscore.js new file mode 100644 index 00000000000000..bc478dbf742418 --- /dev/null +++ b/test/parallel/test-repl-underscore.js @@ -0,0 +1,49 @@ +'use strict'; + +require('../common'); +const assert = require('assert'); +const repl = require('repl'); +const stream = require('stream'); +const inputStream = new stream.PassThrough(); +const outputStream = new stream.PassThrough(); +let output = ''; + +outputStream.on('data', (data) => { + output += data; +}); + +process.on('exit', () => { + const lines = output.trim().split('\n'); + + assert.deepStrictEqual(lines, [ + 'undefined', + '10', + '10', + 'expression assignment to _ now disabled', + '20', + '20', + '30', + '20', + '40', + '40', + '40' + ]); +}); + +const r = repl.start({ + prompt: '', + input: inputStream, + output: outputStream +}); + +r.write('_;\n'); +r.write('x = 10;\n'); +r.write('_;\n'); +r.write('_ = 20;\n'); +r.write('_;\n'); +r.write('y = 30;\n'); +r.write('_;\n'); +r.write('_ = 40;\n'); +r.write('_;\n'); +r.resetContext(); +r.write('_;\n'); From 1dd5e2a1aa761a03f5874fe78c796f4ab1867401 Mon Sep 17 00:00:00 2001 From: Lance Ball Date: Thu, 3 Mar 2016 13:02:31 -0500 Subject: [PATCH 2/6] repl: Reset underscore assignment on .clear Ensures that, when invoking `.clear` in a repl, the underscore assignment behavior is reset, so that its value is again set to the most recently evaluated expression. Additional tests have been added for the behavior of `let` when underscore assignment has been disabled. --- lib/repl.js | 30 +++--- test/parallel/test-repl-underscore.js | 136 ++++++++++++++++++++------ 2 files changed, 124 insertions(+), 42 deletions(-) diff --git a/lib/repl.js b/lib/repl.js index 6da70eba0d5751..ec37ae688a45ce 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -205,7 +205,7 @@ function REPLServer(prompt, self.useGlobal = !!useGlobal; self.ignoreUndefined = !!ignoreUndefined; self.replMode = replMode || exports.REPL_MODE_SLOPPY; - self.setLast = true; + self.underscoreAssigned = false; self.last = undefined; self._inTemplateLiteral = false; @@ -473,7 +473,9 @@ function REPLServer(prompt, // the second argument to this function is there, print it. arguments.length === 2 && (!self.ignoreUndefined || ret !== undefined)) { - if (self.setLast) self.last = ret; + if (!self.underscoreAssigned) { + self.last = ret; + } self.outputStream.write(self.writer(ret) + '\n'); } @@ -547,22 +549,22 @@ REPLServer.prototype.createContext = function() { context.module = module; context.require = require; - const repl = this; + this.underscoreAssigned = false; this.lines = []; this.lines.level = []; // make built-in modules available directly // (loaded lazily) - exports._builtinLibs.forEach(function(name) { + exports._builtinLibs.forEach((name) => { Object.defineProperty(context, name, { - get: function() { + get: () => { var lib = require(name); - repl.last = context[name] = lib; + this.last = context[name] = lib; return lib; }, // allow the creation of other globals with this name - set: function(val) { + set: (val) => { delete context[name]; context[name] = val; }, @@ -572,14 +574,14 @@ REPLServer.prototype.createContext = function() { Object.defineProperty(context, '_', { configurable: true, - get: function() { - return repl.last; + get: () => { + return this.last; }, - set: function(value) { - repl.last = value; - if (repl.setLast) { - repl.setLast = false; - repl.outputStream.write('expression assignment to _ now disabled\n'); + set: (value) => { + this.last = value; + if (!this.underscoreAssigned) { + this.underscoreAssigned = true; + this.outputStream.write('expression assignment to _ now disabled\n'); } } }); diff --git a/test/parallel/test-repl-underscore.js b/test/parallel/test-repl-underscore.js index bc478dbf742418..1a9ba8de764dd9 100644 --- a/test/parallel/test-repl-underscore.js +++ b/test/parallel/test-repl-underscore.js @@ -4,18 +4,37 @@ require('../common'); const assert = require('assert'); const repl = require('repl'); const stream = require('stream'); -const inputStream = new stream.PassThrough(); -const outputStream = new stream.PassThrough(); -let output = ''; -outputStream.on('data', (data) => { - output += data; +var tests = [ + testSloppyMode, + testStrictMode, + testResetContext +]; + +tests.forEach(function(test) { + test(); }); -process.on('exit', () => { - const lines = output.trim().split('\n'); +function testSloppyMode() { + const r = initRepl(repl.REPL_MODE_SLOPPY); + + // cannot use `let` in sloppy mode + r.write(`_; // initial value undefined + var x = 10; // evaluates to undefined + _; // still undefined + y = 10; // evaluates to 10 + _; // 10 from last eval + _ = 20; // explicitly set to 20 + _; // 20 from user input + _ = 30; // make sure we can set it twice and no prompt + _; // 30 from user input + y = 40; // make sure eval doesn't change _ + _; // remains 30 from user input + `); - assert.deepStrictEqual(lines, [ + assertOutput(r.output, [ + 'undefined', + 'undefined', 'undefined', '10', '10', @@ -23,27 +42,88 @@ process.on('exit', () => { '20', '20', '30', - '20', - '40', + '30', '40', - '40' + '30' ]); -}); +} -const r = repl.start({ - prompt: '', - input: inputStream, - output: outputStream -}); +function testStrictMode() { + const r = initRepl(repl.REPL_MODE_STRICT); + + r.write(`_; // initial value undefined + var x = 10; // evaluates to undefined + _; // still undefined + let _ = 20; // use 'let' only in strict mode - evals to undefined + _; // 20 from user input + _ = 30; // make sure we can set it twice and no prompt + _; // 30 from user input + var y = 40; // make sure eval doesn't change _ + _; // remains 30 from user input + function f() { let _ = 50; } // undefined + f(); // undefined + _; // remains 30 from user input + `); + + assertOutput(r.output, [ + 'undefined', + 'undefined', + 'undefined', + 'undefined', + '20', + '30', + '30', + 'undefined', + '30', + 'undefined', + 'undefined', + '30' + ]); +} + +function testResetContext() { + const r = initRepl(repl.REPL_MODE_SLOPPY); + + r.write(`_ = 10; // explicitly set to 10 + _; // 10 from user input + .clear // Clearing context... + _; // remains 10 + x = 20; // but behavior reverts to last eval + _; // expect 20 + `); + + assertOutput(r.output, [ + 'expression assignment to _ now disabled', + '10', + '10', + 'Clearing context...', + '10', + '20', + '20' + ]); +} + +function initRepl(mode, useGlobal) { + const inputStream = new stream.PassThrough(); + const outputStream = new stream.PassThrough(); + outputStream.accum = ''; + + outputStream.on('data', (data) => { + outputStream.accum += data; + }); + + return repl.start({ + input: inputStream, + output: outputStream, + useColors: false, + terminal: false, + prompt: '', + useGlobal: !!useGlobal, + replMode: mode + }); +} -r.write('_;\n'); -r.write('x = 10;\n'); -r.write('_;\n'); -r.write('_ = 20;\n'); -r.write('_;\n'); -r.write('y = 30;\n'); -r.write('_;\n'); -r.write('_ = 40;\n'); -r.write('_;\n'); -r.resetContext(); -r.write('_;\n'); +function assertOutput(output, expected) { + const lines = output.accum.trim().split('\n'); + assert.deepStrictEqual(lines, expected); +} From ca199e25c533ceb69532cd0b99f49ef0c0244120 Mon Sep 17 00:00:00 2001 From: Lance Ball Date: Tue, 8 Mar 2016 14:57:03 -0500 Subject: [PATCH 3/6] doc: Document `_` behavior in REPL. Adds a line of text about the behavior of `_` in the REPL. --- doc/api/repl.markdown | 3 +++ 1 file changed, 3 insertions(+) diff --git a/doc/api/repl.markdown b/doc/api/repl.markdown index e9bf877e8ac06c..9eee709f759347 100644 --- a/doc/api/repl.markdown +++ b/doc/api/repl.markdown @@ -86,6 +86,9 @@ The special variable `_` (underscore) contains the result of the last expression 4 ``` +Explicitly setting `_` will disable this behavior. To re-enable auto-assigning +`_`, you will need to reset the context. To do this, type `.clear`. + The REPL provides access to any variables in the global scope. You can expose a variable to the REPL explicitly by assigning it to the `context` object associated with each `REPLServer`. For example: From 4062e6212854ea320976e4be3cd1cd08727734e9 Mon Sep 17 00:00:00 2001 From: Lance Ball Date: Thu, 10 Mar 2016 22:57:20 -0500 Subject: [PATCH 4/6] repl: Adjust docs for `_` usage, and tighten test. Incorporating comments from @cjhrig. https://github.com/nodejs/node/pull/5535#discussion_r55782643 https://github.com/nodejs/node/pull/5535#discussion_r55783378 --- doc/api/repl.markdown | 3 +-- test/parallel/test-repl-underscore.js | 12 +++--------- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/doc/api/repl.markdown b/doc/api/repl.markdown index 9eee709f759347..f77833ea472932 100644 --- a/doc/api/repl.markdown +++ b/doc/api/repl.markdown @@ -86,8 +86,7 @@ The special variable `_` (underscore) contains the result of the last expression 4 ``` -Explicitly setting `_` will disable this behavior. To re-enable auto-assigning -`_`, you will need to reset the context. To do this, type `.clear`. +Explicitly setting `_` will disable this behavior until the context is reset. The REPL provides access to any variables in the global scope. You can expose a variable to the REPL explicitly by assigning it to the `context` object diff --git a/test/parallel/test-repl-underscore.js b/test/parallel/test-repl-underscore.js index 1a9ba8de764dd9..b718e62d663f59 100644 --- a/test/parallel/test-repl-underscore.js +++ b/test/parallel/test-repl-underscore.js @@ -5,15 +5,9 @@ const assert = require('assert'); const repl = require('repl'); const stream = require('stream'); -var tests = [ - testSloppyMode, - testStrictMode, - testResetContext -]; - -tests.forEach(function(test) { - test(); -}); +testSloppyMode(); +testStrictMode(); +testResetContext(); function testSloppyMode() { const r = initRepl(repl.REPL_MODE_SLOPPY); From 428cef65eeafe61f23c32997ceaaec5d4ae91060 Mon Sep 17 00:00:00 2001 From: Lance Ball Date: Fri, 11 Mar 2016 17:10:18 -0500 Subject: [PATCH 5/6] repl: Test `_` behavior in `REPL_MODE_MAGIC` mode. Add a test based on feedback from @cjihrig. Ensures that when used in 'magic' mode, the behavior of `_` assignment is consistent with other modes and user expectations. --- test/parallel/test-repl-underscore.js | 34 +++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/test/parallel/test-repl-underscore.js b/test/parallel/test-repl-underscore.js index b718e62d663f59..600abe63f061dc 100644 --- a/test/parallel/test-repl-underscore.js +++ b/test/parallel/test-repl-underscore.js @@ -8,6 +8,7 @@ const stream = require('stream'); testSloppyMode(); testStrictMode(); testResetContext(); +testMagicMode(); function testSloppyMode() { const r = initRepl(repl.REPL_MODE_SLOPPY); @@ -75,6 +76,39 @@ function testStrictMode() { ]); } +function testMagicMode() { + const r = initRepl(repl.REPL_MODE_MAGIC); + + r.write(`_; // initial value undefined + x = 10; // + _; // last eval - 10 + let _ = 20; // undefined + _; // 20 from user input + _ = 30; // make sure we can set it twice and no prompt + _; // 30 from user input + var y = 40; // make sure eval doesn't change _ + _; // remains 30 from user input + function f() { let _ = 50; return _; } // undefined + f(); // 50 + _; // remains 30 from user input + `); + + assertOutput(r.output, [ + 'undefined', + '10', + '10', + 'undefined', + '20', + '30', + '30', + 'undefined', + '30', + 'undefined', + '50', + '30' + ]); +} + function testResetContext() { const r = initRepl(repl.REPL_MODE_SLOPPY); From 1471d13fb085cb037a10a8eefa7a4810bae13c2f Mon Sep 17 00:00:00 2001 From: Lance Ball Date: Tue, 15 Mar 2016 12:17:55 -0400 Subject: [PATCH 6/6] repl: Change warning text to a complete sentence. When a REPL user explicitly assigns `_` the warning text now reads, "Expression assignment to _ now disabled.". Also removed an unnecessary parameter from a test helper function. --- lib/repl.js | 2 +- test/parallel/test-repl-underscore.js | 7 +++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/lib/repl.js b/lib/repl.js index ec37ae688a45ce..e95e9b636d7cd2 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -581,7 +581,7 @@ REPLServer.prototype.createContext = function() { this.last = value; if (!this.underscoreAssigned) { this.underscoreAssigned = true; - this.outputStream.write('expression assignment to _ now disabled\n'); + this.outputStream.write('Expression assignment to _ now disabled.\n'); } } }); diff --git a/test/parallel/test-repl-underscore.js b/test/parallel/test-repl-underscore.js index 600abe63f061dc..97fc508ad9eee4 100644 --- a/test/parallel/test-repl-underscore.js +++ b/test/parallel/test-repl-underscore.js @@ -33,7 +33,7 @@ function testSloppyMode() { 'undefined', '10', '10', - 'expression assignment to _ now disabled', + 'Expression assignment to _ now disabled.', '20', '20', '30', @@ -121,7 +121,7 @@ function testResetContext() { `); assertOutput(r.output, [ - 'expression assignment to _ now disabled', + 'Expression assignment to _ now disabled.', '10', '10', 'Clearing context...', @@ -131,7 +131,7 @@ function testResetContext() { ]); } -function initRepl(mode, useGlobal) { +function initRepl(mode) { const inputStream = new stream.PassThrough(); const outputStream = new stream.PassThrough(); outputStream.accum = ''; @@ -146,7 +146,6 @@ function initRepl(mode, useGlobal) { useColors: false, terminal: false, prompt: '', - useGlobal: !!useGlobal, replMode: mode }); }