repl: remove REPLServer.createContext side effects#14331
repl: remove REPLServer.createContext side effects#14331lance wants to merge 0 commit intonodejs:masterfrom lance:14226-repl-create-context
Conversation
|
This is changing the behavior of that method. And though the method was not documented, it was publicly accessible. So, I guess it could break things. I'm adding |
lib/repl.js
Outdated
There was a problem hiding this comment.
I have to admit I don’t see how this is related to the rest of the changes here?
There was a problem hiding this comment.
Well, I suppose only in that it's unnecessary to explicitly assign self.last = undefined because in user-land, myServer.last will be undefined no matter what, until it becomes assigned. I can revert this change if you think keeping it would allow for more clarity.
test/parallel/test-repl-context.js
Outdated
|
I suppose with only one approval, this is GtG, but since it's (maybe) |
|
As a semver major a second CTC approval is required for it to land. |
|
@Fishrock123 @princejwesley looking for a second opinion on this if you can. PTAL. Thanks! |
jasnell
left a comment
There was a problem hiding this comment.
Code change LGTM, still would like @Fishrock123 to look tho.
|
@Fishrock123 any chance you can TAL? Thanks. |
|
I'll try to remember it but I have limited familiarity at this point, would rather another @nodejs/ctc take a look |
|
Ping @cjihrig. Just looking for one more LGTM. Thanks. |
|
@princejwesley thanks - Edit: new linting rules, it seems. Another CI: https://ci.nodejs.org/job/node-test-pull-request/9874/ |
The internal method `REPLServer.createContext()` had unexpected side effects. When called, the value for the `underscoreAssigned` and `lines` properties were reset. This change ensures that those properties are not modified when a context is created. Fixes: #14226 Refs: #7619 PR-URL: #14331 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
|
Landed in: ed1ba45 |
The internal method `REPLServer.createContext()` had unexpected side effects. When called, the value for the `underscoreAssigned` and `lines` properties were reset. This change ensures that those properties are not modified when a context is created. Fixes: nodejs#14226 Refs: nodejs#7619 PR-URL: nodejs#14331 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
The internal method `REPLServer.createContext()` had unexpected side effects. When called, the value for the `underscoreAssigned` and `lines` properties were reset. This change ensures that those properties are not modified when a context is created. Fixes: nodejs/node#14226 Refs: nodejs/node#7619 PR-URL: nodejs/node#14331 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Prince John Wesley <princejohnwesley@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Eliminate unexpected side effects when calling
REPLServer.createContextby moving code which modifies
thisto all exist withinresetContext.Ref: #7619
Fixes: #14226
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
repl