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: do not add 'inspector' property #12656

Merged
merged 1 commit into from May 4, 2017

Conversation

@eugeneo
Copy link
Contributor

commented Apr 25, 2017

'inspector' property is not an official API and should not be published
on process object, where the user may discover it.

This change was extracted from #12263
that will be focused on creating JS bindings.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

inspector: new object is not created and insteaad JS bindings are defined.

lib/internal/bootstrap_node.js Outdated
return;
function installInspectorConsoleIfNeeded(globalConsole) {
var wrappedConsole = NativeModule.require('console');
var inspector = process.binding('inspector');

This comment has been minimized.

Copy link
@TimothyGu

This comment has been minimized.

Copy link
@eugeneo

eugeneo May 1, 2017

Author Contributor

Done, thanks!

src/inspector_agent.cc Outdated



void InspectorWrapConsoleCall(const FunctionCallbackInfo<Value>& info) {

This comment has been minimized.

Copy link
@TimothyGu

TimothyGu Apr 25, 2017

Member

Does this method (and ConsoleCall) need to be written in C++?

This comment has been minimized.

Copy link
@ak239

ak239 Apr 26, 2017

Member

If there is a way to not expose internal JavaScript function in stack trace then it's not necessary.

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 30, 2017

Member

Yeah, there’s Error.captureStackTrace() specifically for that.

This comment has been minimized.

Copy link
@ak239

ak239 May 1, 2017

Member

On inspector side we use native v8 API: v8::StackTrace::CurrentStackTrace instead of JS Error.captureStackTrace and it's really important to have correct top frame for console messages since we use its location as message location.

This comment has been minimized.

Copy link
@eugeneo

eugeneo May 1, 2017

Author Contributor

@addaleax I am not sure how Error.captureStackTrace can be used here.

Inspector records stack traces for all console messages. You can see in in Chrome DevTools where there is a hyperlink to the right of every log record.
Having a native wrapper ensures that Inspector records the place where the native wrapper is called (e.g. the user code, as expected). If you make that wrapper a JS function then that function will be called, meaning there is the same top frame for every console call.

I did a couple prototypes and I do not see how I can remove the native wrapper for the console calls.

@ak239
ak239 approved these changes Apr 26, 2017
src/inspector_agent.cc Outdated



void InspectorWrapConsoleCall(const FunctionCallbackInfo<Value>& info) {

This comment has been minimized.

Copy link
@ak239

ak239 Apr 26, 2017

Member

If there is a way to not expose internal JavaScript function in stack trace then it's not necessary.

call_args.push_back(args[i]);
}

env->inspector_agent()->PauseOnNextJavascriptStatement("Break on start");

This comment has been minimized.

Copy link
@ak239

ak239 Apr 26, 2017

Member

Just idea, maybe we need a way to provide something like blackbox this function method then we can blackbox caller and have just schedule Pause OnNextStatement method.

@eugeneo

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2017

@jasnell
jasnell approved these changes May 4, 2017
inspector: do not add 'inspector' property
'inspector' property is not an official API and should not be published
on process object, where the user may discover it.

This change was extracted from #12263
that will be focused on creating JS bindings.

PR-URL: #12656
Reviewed-By: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>

@eugeneo eugeneo merged commit 3f48ab3 into nodejs:master May 4, 2017

@eugeneo eugeneo deleted the eugeneo:no-inspector-object branch May 4, 2017

@eugeneo

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2017

Landed as 3f48ab3

anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
inspector: do not add 'inspector' property
'inspector' property is not an official API and should not be published
on process object, where the user may discover it.

This change was extracted from nodejs#12263
that will be focused on creating JS bindings.

PR-URL: nodejs#12656
Reviewed-By: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
@jasnell jasnell referenced this pull request May 11, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 18, 2017

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@sam-github

This comment has been minimized.

Copy link
Member

commented Jul 25, 2017

@eugeneo most (all?) of the inspector work on master isn't landing on 6.x, should it? Is there any reason we should let the inspector on 6.x and 8.x (and later) diverge? For example, is the underlying V8 inspector implementations too different for us to be able to keep them in sync?

cc: @nodejs/lts

@eugeneo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2017

@sam-github there had been divergence in the command line options, console output. Also, there was JS API added. My understanding is that all of that is considered an API breaking change and can't be backported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.