repl: assign underscore fix #3737
Conversation
| @@ -545,6 +545,9 @@ REPLServer.prototype.createContext = function() { | |||
| }); | |||
| }); | |||
|
|
|||
| // Refer: https://github.com/nodejs/node/pull/3729#issuecomment-155460861 | |||
There was a problem hiding this comment.
Instead of referring to an issue, how about just explaining why we're doing this.
There was a problem hiding this comment.
Adding here as well.
There was a problem hiding this comment.
+1 to @cjihrig's comment. The reference to the issue is unnecessary, I think.
|
In the docs, where it says |
thefourtheye
force-pushed
the
repl-assign-underscore-fix
branch
from
c5273ff
to
3d8bb15
Compare
|
@cjihrig Updated as per the suggestions. PTAL. |
| @@ -219,6 +219,10 @@ The special variable `_` (underscore) contains the result of the last expression | |||
| > _ += 1 | |||
| 4 | |||
|
|
|||
| *NOTE*: Explicitly assigning a value to `_` in the REPL can produce unexpected | |||
| results. Also, attempting to create `_` as a `const` variable in REPL will | |||
There was a problem hiding this comment.
Can you add "the" between "in" and "REPL."
|
A couple small comments, but LGTM. Once the CI is fully available, feel free to squash this and run the tests. |
| // Refer: https://github.com/nodejs/node/pull/3729#issuecomment-155460861 | ||
| // REPL stores the result of the last evaluated expression in `context`'s `_`. | ||
| // But, in REPL, if `_` is defined as a `const`, then it will break the REPL. | ||
| // So, we define `_` first, so that later redefintions will fail. |
|
LGTM sans a few minor comments. |
|
@thefourtheye ... can you please rebase and update? |
thefourtheye
force-pushed
the
repl-assign-underscore-fix
branch
from
63b27a6
to
71fe0a9
Compare
As `_` is not defined in REPL's context, when it is defined as `const`, it breaks REPL, as it tries to store the result of the last evaluated expression in `_`. This patch makes sure that `_` is pre-defined in REPL's context, so that if users define it again, they will get error. Refer: nodejs#3729 Refer: nodejs#3704
If the `_` is redefined as `const` in REPL, it will break the REPL, as REPL will store the result of the last evaluated expression in `_`. This patch has a test to make sure that the REPL doesn't allow redefining `_` as `const`, also still assiging values to `_` is permitted. Refer: nodejs#3729 Refer: nodejs#3704
When users assign a value to `_` in REPL, it is prone to unexpected results or messing up with REPL's internals. For example, nodejs#3704. This patch issues a warning about the same.
thefourtheye
force-pushed
the
repl-assign-underscore-fix
branch
from
71fe0a9
to
48d79a4
Compare
|
Sorry for the delay. I updated and rebased now. PTAL. |
|
LGTM |
|
Closing given that #5535 landed. |
As
_is not defined in REPL's context, when it is defined asconst,it breaks REPL, as it tries to store the result of the last evaluated
expression in
_. This patch makes sure that_is pre-defined inREPL's context, so that if users define it again, they will get error.
This patch has a test to make sure that the REPL doesn't allow
redefining
_asconst, also still assiging values to_ispermitted.
Refer: #3729
Refer: #3704
cc @jasnell @cjihrig @targos @SamuelMarks @Fishrock123