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

inspector,vm: remove --eval wrapper #25832

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

Report the actual source code when running with --eval and
--inspect-brk, by telling the vm module to break on the
first line of the script being executed rather than wrapping
the source code in a function.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Report the actual source code when running with `--eval` and
`--inspect-brk`, by telling the vm module to break on the
first line of the script being executed rather than wrapping
the source code in a function.
@addaleax addaleax added vm Issues and PRs related to the vm subsystem. inspector Issues and PRs related to the V8 inspector protocol labels Jan 30, 2019
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 30, 2019
filename: ${JSON.stringify(name)},
displayErrors: true,
[kVmBreakFirstLineSymbol]: ${!!breakFirstLine}
});\n`;
Copy link
Member

Choose a reason for hiding this comment

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

@addaleax The kVmBreakFirstLineSymbol looks pretty handy. Is there a reason to keep it hidden and not exposed as a user facing option like a boolean breakFirstLineSymbol options property?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jdalton Err, it’s less work for this PR? 😄 I’m not sure, but I guess it also only works while the inspector is enabled, similar to debugger; statements?

Somebody from @nodejs/v8-inspector might know more.

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

Supersedes #24946 (which uses debugger;), this just removes the wrapper entirely so the source panel is cleaner in the inspector UI.

@addaleax
Copy link
Member Author

@joyeecheung Sorry, I wasn’t aware of that PR – it looks like that mostly consists of test changes that we could still apply, right?

@addaleax
Copy link
Member Author

addaleax commented Feb 1, 2019

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 1, 2019
@addaleax
Copy link
Member Author

addaleax commented Feb 3, 2019

@addaleax
Copy link
Member Author

addaleax commented Feb 3, 2019

Landed in cca897e

@addaleax addaleax closed this Feb 3, 2019
@addaleax addaleax deleted the vm-break-first-line branch February 3, 2019 19:41
addaleax added a commit that referenced this pull request Feb 3, 2019
Report the actual source code when running with `--eval` and
`--inspect-brk`, by telling the vm module to break on the
first line of the script being executed rather than wrapping
the source code in a function.

PR-URL: #25832
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
addaleax added a commit that referenced this pull request Feb 3, 2019
Report the actual source code when running with `--eval` and
`--inspect-brk`, by telling the vm module to break on the
first line of the script being executed rather than wrapping
the source code in a function.

PR-URL: #25832
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@targos targos mentioned this pull request Feb 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol lib / src Issues and PRs related to general changes in the lib or src directory. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants