repl: Assignment of _ allowed with warning by lance · Pull Request #5535 · nodejs/node (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation44 Commits6 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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.

mscdex added the repl

Issues and PRs related to the REPL subsystem.

label

Mar 2, 2016

cjihrig

@@ -471,7 +473,7 @@ function REPLServer(prompt,
// the second argument to this function is there, print it.
arguments.length === 2 &&
(!self.ignoreUndefined \|
self.context._ = ret;
if (self.setLast) self.last = ret;

Choose a reason for hiding this comment

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

Can you split this across two lines please.

jasnell added the semver-major

PRs that contain breaking changes and should be released in the next major version.

label

Mar 3, 2016

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.

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

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.

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!).

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

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

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

Totally understand. Thanks for hearing me out.

LGTM

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

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

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

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

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

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

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

cjihrig

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

Choose a reason for hiding this comment

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

Can you phrase this as:

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

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.

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 pushed a commit that referenced this pull request

Mar 18, 2016

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

jasnell added a commit that referenced this pull request

Apr 26, 2016