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.

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

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

Closed
wants to merge 1 commit into from

Conversation

@jkrems
Copy link
Contributor

commented Mar 15, 2017

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

Jan Krems
repl: Empty command should be sent to eval function
This fixes a regression introduced in #6171
@jkrems

This comment has been minimized.

Copy link
Contributor Author

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 referenced this pull request Mar 15, 2017
2 of 2 tasks complete
@Fishrock123

This comment has been minimized.

@jkrems

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2017

🍏

@addaleax
Copy link
Member

left a comment

LGTM with a suggestion


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

This comment has been minimized.

Copy link
@addaleax

addaleax Mar 15, 2017

Member

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

This comment has been minimized.

Copy link
@jkrems

jkrems Mar 15, 2017

Author Contributor

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 added a commit that referenced this pull request Mar 17, 2017
repl: Empty command should be sent to eval function
This fixes a regression introduced in #6171

PR-URL: #11871
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell

This comment has been minimized.

Copy link
Member

commented Mar 17, 2017

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

@jasnell

This comment has been minimized.

Copy link
Member

commented Mar 17, 2017

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

@jkrems jkrems deleted the jkrems:repl-enter-eval branch Mar 17, 2017

addaleax added a commit to addaleax/node that referenced this pull request Mar 20, 2017
test: minor fixups for REPL eval tests
The `process.on("exit")` event handlers are unnecessary, so it’s okay
to drop them.

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

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

PR-URL: #11946
Ref: #11871
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@jasnell jasnell referenced this pull request Apr 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.