Skip to content

repl: remove REPLServer.createContext side effects#14331

Closed
lance wants to merge 0 commit intonodejs:masterfrom
lance:14226-repl-create-context
Closed

repl: remove REPLServer.createContext side effects#14331
lance wants to merge 0 commit intonodejs:masterfrom
lance:14226-repl-create-context

Conversation

@lance
Copy link
Copy Markdown
Member

@lance lance commented Jul 17, 2017

Eliminate unexpected side effects when calling REPLServer.createContext
by moving code which modifies this to all exist within resetContext.

Ref: #7619
Fixes: #14226

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

repl

@lance lance added the repl Issues and PRs related to the REPL subsystem. label Jul 17, 2017
@Trott
Copy link
Copy Markdown
Member

Trott commented Jul 18, 2017

@addaleax, @Fishrock123 @princejwesley

@lance
Copy link
Copy Markdown
Member Author

lance commented Jul 18, 2017

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

@lance
Copy link
Copy Markdown
Member Author

lance commented Jul 18, 2017

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 semver-major label for now, in the interest of conservatism.

@lance lance added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 18, 2017
addaleax
addaleax approved these changes Jul 20, 2017
View reviewed changes
lib/repl.js Outdated
Copy link
Copy Markdown
Member

@addaleax addaleax Jul 20, 2017

Choose a reason for hiding this comment

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

I have to admit I don’t see how this is related to the rest of the changes here?

Copy link
Copy Markdown
Member Author

@lance lance Jul 20, 2017

Choose a reason for hiding this comment

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

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
Copy link
Copy Markdown
Member

@addaleax addaleax Jul 20, 2017

Choose a reason for hiding this comment

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

This is a typo (_), right?

Copy link
Copy Markdown
Member Author

@lance lance Jul 20, 2017

Choose a reason for hiding this comment

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

Yes - typo...

@lance
Copy link
Copy Markdown
Member Author

lance commented Jul 27, 2017

I suppose with only one approval, this is GtG, but since it's (maybe) semver-major another approval would make me more comfortable landing this. @Fishrock123 @princejwesley?

@jasnell jasnell requested a review from Fishrock123 July 31, 2017 19:16
@jasnell
Copy link
Copy Markdown
Member

jasnell commented Jul 31, 2017

As a semver major a second CTC approval is required for it to land.

@lance
Copy link
Copy Markdown
Member Author

lance commented Aug 2, 2017

@Fishrock123 @princejwesley looking for a second opinion on this if you can. PTAL. Thanks!

jasnell
jasnell approved these changes Aug 3, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Code change LGTM, still would like @Fishrock123 to look tho.

@lance
Copy link
Copy Markdown
Member Author

lance commented Aug 15, 2017

@Fishrock123 any chance you can TAL? Thanks.

@Fishrock123 Fishrock123 self-assigned this Aug 15, 2017
@Fishrock123
Copy link
Copy Markdown
Contributor

Fishrock123 commented Aug 15, 2017

I'll try to remember it but I have limited familiarity at this point, would rather another @nodejs/ctc take a look

@lance
Copy link
Copy Markdown
Member Author

lance commented Aug 16, 2017

Ping @cjihrig. Just looking for one more LGTM. Thanks.

@lance
Copy link
Copy Markdown
Member Author

lance commented Aug 29, 2017 •
edited
Loading

@princejwesley thanks - one more CI for good measure <https://ci.nodejs.org/job/node-test-pull-request/9873/

Edit: new linting rules, it seems. Another CI: https://ci.nodejs.org/job/node-test-pull-request/9874/

lance added a commit that referenced this pull request Aug 30, 2017
@lance
repl: remove REPLServer.createContext side effects
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: #14226
Refs: #7619

PR-URL: #14331
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Prince John Wesley <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@lance
Copy link
Copy Markdown
Member Author

lance commented Aug 30, 2017

Landed in: ed1ba45

@lance lance closed this Aug 30, 2017
@lance lance deleted the 14226-repl-create-context branch August 30, 2017 20:50
cjihrig pushed a commit to cjihrig/node that referenced this pull request Aug 31, 2017
@lance @cjihrig
repl: remove REPLServer.createContext side effects
7f6f5fd 
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Prince John Wesley <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 5, 2017
@lance @addaleax
repl: remove REPLServer.createContext side effects
30a71f8 
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 <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Prince John Wesley <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@lance lance mentioned this pull request Oct 16, 2017
Closed
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@Fishrock123 Fishrock123 Awaiting requested review from Fishrock123

+1 more reviewer

@princejwesley princejwesley princejwesley approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Fishrock123 Fishrock123

Labels

repl Issues and PRs related to the REPL subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@lance @Trott @jasnell @Fishrock123 @addaleax @princejwesley @BridgeAR