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: Empty command should be sent to eval function #11871

Closed
wants to merge 1 commit into from
Closed

repl: Empty command should be sent to eval function #11871

wants to merge 1 commit into from

Conversation

jkrems
Copy link
Contributor

@jkrems jkrems commented Mar 15, 2017 •
edited
Loading

This fixes a regression introduced in #6171

The command line debugger relies on the previous behavior. This adds a regression test and restores it.

Checklist
  • make -j4 test passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

repl

repl: Empty command should be sent to eval function
7683ef3 
This fixes a regression introduced in nodejs#6171
@nodejs-github-bot nodejs-github-bot added the repl Issues and PRs related to the REPL subsystem. label Mar 15, 2017
@jkrems
Copy link
Contributor Author

jkrems commented Mar 15, 2017

Verified against node-inspect:

> ../node/node cli.js examples/empty.js
< Debugger listening on port 9229.
< Warning: This is an experimental feature and could change at any time.
< To start debugging, open the following URL in Chrome:
<     chrome-devtools://devtools/bundled/inspector.html?experiments=true&v8only=true&ws=127.0.0.1:9229/2f87b9e8-e570-4878-b33e-4858c0d7f7a0
< Debugger attached.
break in examples/empty.js:2
  1 (function (exports, require, module, __filename, __dirname) {
> 2 });
debug> ["hello", "world"].join(" ")
'hello world'
debug>
'hello world'
debug>

On current master, the 2nd 'hello world' isn't printed.

@jkrems jkrems mentioned this pull request Mar 15, 2017
Closed
2 tasks
@Fishrock123
Copy link
Contributor

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

@jkrems
Copy link
Contributor Author

jkrems commented Mar 15, 2017

🍏

addaleax
addaleax approved these changes Mar 15, 2017
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM with a suggestion

test/parallel/test-repl-empty.js

process.on('exit', () => {
assert(evalCalledWithExpectedArgs);
});
Copy link
Member

Choose a reason for hiding this comment

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

I think you don’t need for the process to exit, the eval callback should be called synchronousl

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can simplify the test file, just wrote it this way to match the existing ones (cp test/parallel/test-repl-{eval,empty}.js).

jasnell pushed a commit that referenced this pull request Mar 17, 2017
@jasnell
repl: Empty command should be sent to eval function
c7b6016 
This fixes a regression introduced in #6171

PR-URL: #11871
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@jasnell
Copy link
Member

jasnell commented Mar 17, 2017

Landed in c7b6016.
The additional simplifications suggested by @addaleax can be done in a separate PR :-)

@jasnell jasnell closed this Mar 17, 2017
@jasnell
Copy link
Member

jasnell commented Mar 17, 2017

Depends on a prior semver-major commit. Marking do-not-land on lower versions.

@jkrems jkrems deleted the repl-enter-eval branch March 17, 2017 16:20
addaleax added a commit to addaleax/node that referenced this pull request Mar 20, 2017
@addaleax
test: minor fixups for REPL eval tests
3859673 
The `process.on("exit")` event handlers are unnecessary, so it’s okay
to drop them.

Ref: nodejs#11871
@addaleax addaleax mentioned this pull request Mar 20, 2017
Closed
3 tasks
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
@jungx098
repl: Empty command should be sent to eval function
fb43588 
This fixes a regression introduced in nodejs#6171

PR-URL: nodejs#11871
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
jasnell pushed a commit that referenced this pull request Mar 22, 2017
@addaleax @jasnell
test: minor fixups for REPL eval tests
64d0a73 
The `process.on("exit")` event handlers are unnecessary, so it’s okay
to drop them.

PR-URL: #11946
Ref: #11871
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@jasnell jasnell mentioned this pull request Apr 4, 2017
Closed
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

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jkrems @Fishrock123 @jasnell @addaleax @cjihrig @nodejs-github-bot