inspector: do not add 'inspector' property#12656
inspector: do not add 'inspector' property#12656eugeneo merged 1 commit intonodejs:masterfrom eugeneo:no-inspector-object
Conversation
nodejs-github-bot
added
c++
lib/internal/bootstrap_node.js
Outdated
src/inspector_agent.cc
Outdated
There was a problem hiding this comment.
Does this method (and ConsoleCall) need to be written in C++?
There was a problem hiding this comment.
If there is a way to not expose internal JavaScript function in stack trace then it's not necessary.
There was a problem hiding this comment.
Yeah, there’s Error.captureStackTrace() specifically for that.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
src/inspector_agent.cc
Outdated
There was a problem hiding this comment.
If there is a way to not expose internal JavaScript function in stack trace then it's not necessary.
src/inspector_agent.cc
Outdated
There was a problem hiding this comment.
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.
'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>
|
Landed as 3f48ab3 |
'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>
|
Should this be backported to |
|
@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 |
|
@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. |
9 participants
'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), orvcbuild test(Windows) passesAffected core subsystem(s)
inspector: new object is not created and insteaad JS bindings are defined.