Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

repl: remove REPLServer.createContext side effects #14331

Closed
wants to merge 0 commits into from
Closed

repl: remove REPLServer.createContext side effects #14331

wants to merge 0 commits into from

Conversation

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

Trott commented Jul 18, 2017

@addaleax, @Fishrock123 @princejwesley

@lance
Copy link
Member Author

lance commented Jul 18, 2017

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

@lance
Copy link
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
@@ -150,7 +150,6 @@ function REPLServer(prompt,
self.ignoreUndefined = !!ignoreUndefined;
self.replMode = replMode || exports.REPL_MODE_SLOPPY;
self.underscoreAssigned = false;
self.last = undefined;
Copy link
Member

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
Member Author

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
assert.ok(!server.underscoreAssigned);
assert.strictEqual(server.lines.length, 0);

// an assignment to '-' in the repl server
Copy link
Member

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
Member Author

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
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
Member

jasnell commented Jul 31, 2017

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

@lance
Copy link
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
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
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
Contributor

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
Member Author

lance commented Aug 16, 2017

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

@lance
Copy link
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
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

@BridgeAR BridgeAR BridgeAR approved these changes

@princejwesley princejwesley princejwesley approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

@Fishrock123 Fishrock123 Awaiting requested review from Fishrock123

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