repl: Assignment of _ allowed with warning #5535

lance · 2016-03-02T22:05:30Z

Pull Request check-list

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)?

If acceptable, I can add a PR for the REPL documentation.

Affected core subsystem(s)

repl

Description of change

This commit addresses #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.

cjihrig · 2016-03-02T22:32:38Z

lib/repl.js

@@ -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;


Can you split this across two lines please.

silverwind · 2016-03-02T23:37:57Z

Thanks for doing this!

jasnell · 2016-03-03T18:14:32Z

hmmm... I'm not yet convinced that this is a good idea. Perhaps what we ultimately need is an alternative to _ for holding the result of the last expression and deprecate the use of _ for this purpose? Not sure what else would work tho.

lance · 2016-03-03T18:27:57Z

Added a commit to address @silverwind and @cjihrig comments.

lance · 2016-03-03T21:33:56Z

@jasnell I think the existing behavior definitely needs to change. Or at a minimum the documentation needs to clearly reflect how it currently works, since underscore and lodash are so widely used and the defacto naming convention for these packages is _. In any case, I think there's pretty widespread agreement that the combination of existing behavior and documentation now is not ideal.

An alternative to this implementation would be to allow for environment variables or other initializing values to determine the behavior. E.g.

var repl = require('repl');
var r = repl.start( { assignUnderscore: false } );

Or on the command line:

$ NODE_REPL_ASSIGN_UNDERSCORE=false node

When not set to false, if _ is explicitly set by assignment, then throw an exception.

Wrt an alternative to _ for holding the result of the last expression, I think @bnoordhuis summed it up well: #5431 (comment). In my opinion, the default behavior of assigning _ to the last evaluated expression is time-honored and shouldn't change as a default, but there does need to be a way to set _ and have the repl behave reasonably.

jasnell · 2016-03-03T21:58:09Z

I think I'd be far more comfortable with the approach of disallowing assignment to _ unless a command line flag or option was set explicitly. Doing so would discourage overloading _ even if it does run counter to the lodash/underscore usage conventions.

rvagg · 2016-03-03T22:18:25Z

I don't really have much of an opinion on this cause I've never had a use for _, but my very uninformed guess is that even those few people who know what _, not many of them actually care about it or actively use it. I'm testing that here: https://twitter.com/NodeSource/status/705514998341226496 (I'm not suggesting that the outcome of a Twitter poll should determine the path forward here at all, it should be very interesting though!).

lance · 2016-03-04T18:45:38Z

@jasnell by "disallowing", do you mean just roll with the existing behavior (no warning, overwritten on next expression)? Or do you mean throwing an exception, or displaying a warning?

lance · 2016-03-04T23:24:29Z

@jasnell after giving this more thought, I have to disagree with you. Look at this from a usability perspective. Adding an option that is off by default doesn't help people who get bit by this - underscore/lodash users. They'll spend at least a little bit of time before they discover the option. Issuing a warning in the REPL and telling the user to start over and set the option is one approach to eliminating this pain. But I think another, better, approach is to let users set it, but warn them it won't be further set by the REPL.

I can submit an alternative PR that handles it with an option/env, but I can't see the justification for doing it that way. Assuming the majority of people don't know the option exists, how many people will helped with this PR vs. how many people will be inconvenienced by adding it as an option. Sure, twitter polls shouldn't be the reason for a change, but @rvagg's poll definitely supports this PR.

Can you explain what problems you see with this approach? Users who already know about _ behavior, whether they use it or not, won't be affected because they already are not in the habit of setting it. Users who don't know about _ behavior expect it to be available for lodash or whatever, and will have a better/smoother experience.

jasnell · 2016-03-04T23:57:01Z

I can live with this. I simply tend to take a conservative approach to
changing existing behaviors.
On Mar 4, 2016 3:24 PM, "Lance Ball" notifications@github.com wrote:

@jasnell https://github.com/jasnell after giving this more thought, I
have to disagree with you. Look at this from a usability perspective.
Adding an option that is off by default doesn't help people who get bit by
this - underscore/lodash users. They'll spend at least a little bit of time
before they discover the option. Issuing a warning in the REPL and telling
the user to start over and set the option is one approach to eliminating
this pain. But I think another, better, approach is to let users set it,
but warn them it won't be further set by the REPL.

I can submit an alternative PR that handles it with an option/env, but I
can't see the justification for doing it that way. Assuming the majority of
people don't know the option exists, how many people will helped with this
PR vs. how many people will be inconvenienced by adding it as an option.
Sure, twitter polls shouldn't be the reason for a change, but @rvagg
https://github.com/rvagg's poll definitely supports this PR.

Can you explain what problems you see with this approach? Users who
already know about _ behavior, whether they use it or not, won't be
affected because they already are not in the habit of setting it. Users who
don't know about _ behavior expect it to be available for lodash or
whatever, and will have a better/smoother experience.

—
Reply to this email directly or view it on GitHub
#5535 (comment).

lance · 2016-03-05T02:10:01Z

Totally understand. Thanks for hearing me out.

silverwind · 2016-03-07T13:31:47Z

LGTM

CI seems currently down, will start one once it's up again.

Fishrock123 · 2016-03-07T16:53:19Z

Haven't there been like 3 PRs for this? What is the status of the others?

lance · 2016-03-07T17:19:53Z

@Fishrock123 I have seen this #5438 which was closed. It did not handle assignment, really, other than a warning. And this #5438 which addresses assigning _ as a const, and the behavior is a subset of this PR. If there are others, I am unaware of them.

tuures · 2016-03-10T08:50:27Z

Hello! Lodash is the most depended upon npm package and I'd love to see the current behaviour change. Either by making the variable name configurable or by making it more compatible like this PR. 👍

silverwind · 2016-03-10T10:27:30Z

CI is still giving me 504 Gateway Time-out. @nodejs/build anyone know what's up?

rvagg · 2016-03-10T11:01:15Z

@silverwind that's a Jenkins thing .. I think it happens on active jobs that have lots of history. Keep trying and it should load. I'm not sure what we can do about it though.

silverwind · 2016-03-10T11:10:45Z

@rvagg indeed, suddenly it loaded instantly. Here we go:

https://ci.nodejs.org/job/node-test-pull-request/1894/

jbergstroem · 2016-03-10T11:25:28Z

I'll have a housecleaning at jenkins hq done within the week.

silverwind · 2016-03-10T11:52:50Z

CI is green, @jasnell @cjihrig can you sign this off too?

jasnell · 2016-03-10T15:29:25Z

LGTM

cjihrig · 2016-03-11T02:33:56Z

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`.


Can you phrase this as:

Explicitly setting _ will disable this behavior until the context is reset.

cjihrig · 2016-03-11T02:40:45Z

A test of magic mode that mixes let and var together would be good to have.

This commit addresses #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.

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.

Adds a line of text about the behavior of `_` in the REPL.

Incorporating comments from @cjhrig. #5535 (comment) #5535 (comment)

@cjihrig

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.

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.

silverwind · 2016-03-18T19:59:55Z

https://ci.nodejs.org/job/node-test-pull-request/1964/

silverwind · 2016-03-18T20:36:50Z

Landed in ad8257f, thanks!

This commit addresses #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. PR-URL: #5535 Fixes: #5431 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: James M Snell <jasnell@gmail.com>

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * OS X * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7 [#6402](#6402). * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

mscdex added the repl Issues and PRs related to the REPL subsystem. label Mar 2, 2016

cjihrig reviewed Mar 2, 2016
View reviewed changes

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 3, 2016

cjihrig reviewed Mar 11, 2016
View reviewed changes

lance added 6 commits March 15, 2016 12:19

doc: Document _ behavior in REPL.

ca199e2

Adds a line of text about the behavior of `_` in the REPL.

repl: Adjust docs for _ usage, and tighten test.

4062e62

Incorporating comments from @cjhrig. #5535 (comment) #5535 (comment)

repl: Test _ behavior in REPL_MODE_MAGIC mode.

428cef6

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.

repl: Change warning text to a complete sentence.

1471d13

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.

silverwind closed this Mar 18, 2016

jasnell mentioned this pull request Mar 22, 2016

repl: assign underscore fix #3737

Closed

jasnell mentioned this pull request Apr 19, 2016

What is new in v6? #6264

Closed

jasnell mentioned this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release #6383

Closed

chocolateboy mentioned this pull request Apr 3, 2017

use a different variable name for lodash in readme example victorb/trymodule#23

Merged

abernix mentioned this pull request Oct 31, 2017

[1.6] Starting meteor shell overwrite _ in the main server meteor/meteor#9276

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

repl: Assignment of _ allowed with warning #5535

repl: Assignment of _ allowed with warning #5535

lance commented Mar 2, 2016

cjihrig Mar 2, 2016

silverwind commented Mar 2, 2016

jasnell commented Mar 3, 2016

lance commented Mar 3, 2016

lance commented Mar 3, 2016

jasnell commented Mar 3, 2016

rvagg commented Mar 3, 2016

lance commented Mar 4, 2016

lance commented Mar 4, 2016

jasnell commented Mar 4, 2016

lance commented Mar 5, 2016

silverwind commented Mar 7, 2016

Fishrock123 commented Mar 7, 2016

lance commented Mar 7, 2016

tuures commented Mar 10, 2016

silverwind commented Mar 10, 2016

rvagg commented Mar 10, 2016

silverwind commented Mar 10, 2016

jbergstroem commented Mar 10, 2016

silverwind commented Mar 10, 2016

jasnell commented Mar 10, 2016

cjihrig Mar 11, 2016

cjihrig commented Mar 11, 2016

silverwind commented Mar 18, 2016

silverwind commented Mar 18, 2016

repl: Assignment of _ allowed with warning #5535

repl: Assignment of _ allowed with warning #5535

Conversation

lance commented Mar 2, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

cjihrig Mar 2, 2016

Choose a reason for hiding this comment

silverwind commented Mar 2, 2016

jasnell commented Mar 3, 2016

lance commented Mar 3, 2016

lance commented Mar 3, 2016

jasnell commented Mar 3, 2016

rvagg commented Mar 3, 2016

lance commented Mar 4, 2016

lance commented Mar 4, 2016

jasnell commented Mar 4, 2016

lance commented Mar 5, 2016

silverwind commented Mar 7, 2016

Fishrock123 commented Mar 7, 2016

lance commented Mar 7, 2016

tuures commented Mar 10, 2016

silverwind commented Mar 10, 2016

rvagg commented Mar 10, 2016

silverwind commented Mar 10, 2016

jbergstroem commented Mar 10, 2016

silverwind commented Mar 10, 2016

jasnell commented Mar 10, 2016

cjihrig Mar 11, 2016

Choose a reason for hiding this comment

cjihrig commented Mar 11, 2016

silverwind commented Mar 18, 2016

silverwind commented Mar 18, 2016