inspector: add inspector console support for native Console by alexkozy · Pull Request #7988 · nodejs/node

alexkozy · 2016-08-05T21:18:17Z

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

console

Description of change

When node is running with --inspect flag, default console.log,
console.warn and other methods call inspector console methods in
addition to current behaviour (dump formatted message to stderr
and stdout). Inspector console methods forward message to DevTools and
show its in DevTools Console with DevTools formatters. Inspector console
methods not presented on Node console will be added into it.

Only own methods on global.console object will be changed while
debugging session. User still able to redefine it, use console.Console
or change original methods on Console.prototype.

alexkozy · 2016-08-05T21:23:50Z

/cc @ofrobots @eugeneo

jasnell · 2016-08-05T21:24:03Z

@bnoordhuis @ofrobots @trevnorris ...

eugeneo · 2016-08-05T21:36:10Z

src/node.cc

I would like to suggest moving this block (and 2 functions above) into inspector_agent.{h,cc} - this way there will be less conditional compilation to maintain.

alexkozy · 2016-08-05T22:07:55Z

/cc @dgozman

mscdex · 2016-08-05T22:54:12Z

Wouldn't it be possible to instead store copies of the original console functions, which would allow us to avoid having to modify console.js at all? Having to make those console.js changes just for inspector makes me cringe.

alexkozy · 2016-08-06T02:01:15Z

I addressed your comments and moved all changes into inspector_agent.cc and bootstrap_node.js.
Please take another look!

dgozman · 2016-08-06T02:48:04Z

src/inspector_agent.cc

Let's check that arguments are functions here.

alexkozy · 2016-08-06T21:10:13Z

Addressed all comments, please take another look!

bnoordhuis · 2016-08-07T12:05:43Z

lib/internal/bootstrap_node.js

Still a s/sequentally/sequentially/ typo here.

bnoordhuis · 2016-08-07T12:10:19Z

src/inspector_agent.cc

Nit: operator should go on previous line.

alexkozy · 2016-08-07T17:31:48Z

addressed comments, please take another look.

bnoordhuis · 2016-08-07T20:27:44Z

src/inspector_agent.cc

Nit: arguments should line up.

bnoordhuis · 2016-08-07T20:31:10Z

LGTM with a suggestion and a nit. CI: https://ci.nodejs.org/job/node-test-pull-request/3559/

alexkozy · 2016-08-07T23:23:44Z

Thanks! All done. It looks like freebsd test failure doesn't relate to this pull request.

jasnell · 2016-08-08T14:51:36Z

LGTM

ofrobots · 2016-08-08T15:14:32Z

lib/internal/bootstrap_node.js

I would prefer if inspector wasn't a public property on process (except during startup). Could you get away with deleting process.inspector here?

I can remove process.inspector here. Done.

alexkozy · 2016-08-08T16:25:48Z

All done. Please take another look!

ofrobots · 2016-08-08T22:44:57Z

LGTM. New CI: https://ci.nodejs.org/job/node-test-pull-request/3580/.

trevnorris · 2016-08-09T16:40:37Z

lib/internal/bootstrap_node.js

grammar nit: "then wrap these two methods..."

Also, "will preserve the original stack"

alexkozy · 2016-08-09T16:51:06Z

Thanks for review.
Fixed grammar.

jasnell · 2016-08-09T16:55:02Z

src/inspector_agent.cc

Would prefer the error message to be a bit more descriptive

jasnell · 2016-08-09T16:55:57Z

LGTM with a nit

alexkozy · 2016-08-09T17:04:45Z

Done.

ofrobots · 2016-08-10T23:22:53Z

One more CI: https://ci.nodejs.org/job/node-test-pull-request/3616/

alexkozy · 2016-08-11T00:04:19Z

This time it was failed on https://ci.nodejs.org/job/node-compile-windows/3631/label=win-vs2015/console .
I compare output with previous successful build and it looks like difference at the end of output:
c:\workspace\node-compile-windows\label\win-vs2015>exit 0
FATAL: java.io.IOException: Connection aborted: org.jenkinsci.remoting.nio.NioChannelHub$MonoNioTransport@6034828e[name=test-rackspace-win2012r2-x64-6] ...

It looks like it doesn't relate to my change, does it?

ofrobots · 2016-08-11T00:08:40Z

That seems to be infrastructure related. Windows did pass on the previous build. I think this is good to land. I will do so tomorrow.

When node is running with --inspect flag, default console.log, console.warn and other methods call inspector console methods in addition to current behaviour (dump formatted message to stderr and stdout). Inspector console methods forward message to DevTools and show its in DevTools Console with DevTools formatters. Inspector console methods not presented on Node console will be added into it. Only own methods on global.console object will be changed while debugging session. User still able to redefine it, use console.Console or change original methods on Console.prototype.

alexkozy · 2016-08-12T00:35:41Z

I've rebased this pull request to resolve conflicts.

ofrobots · 2016-08-12T00:52:21Z

Thanks, landed as f6070a1.

When node is running with --inspect flag, default console.log, console.warn and other methods call inspector console methods in addition to current behaviour (dump formatted message to stderr and stdout). Inspector console methods forward message to DevTools and show up in DevTools Console with DevTools formatters. Inspector console methods not present on Node console will be added into it. Only own methods on global.console object will be changed while in a debugging session. User are still able to redefine it, use console.Console or change original methods on Console.prototype. PR-URL: #7988 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: jasnell - James M Snell <jasnell@gmail.com> Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>

ofrobots · 2016-08-12T00:56:24Z

Looks great, btw:

// ...
http.get('http://www.google.com/index.html', (res) => {
  console.log(res);
  res.resume();
});

When node is running with --inspect flag, default console.log, console.warn and other methods call inspector console methods in addition to current behaviour (dump formatted message to stderr and stdout). Inspector console methods forward message to DevTools and show up in DevTools Console with DevTools formatters. Inspector console methods not present on Node console will be added into it. Only own methods on global.console object will be changed while in a debugging session. User are still able to redefine it, use console.Console or change original methods on Console.prototype. PR-URL: #7988 Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: jasnell - James M Snell <jasnell@gmail.com> Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 5, 2016

eugeneo reviewed Aug 5, 2016
View reviewed changes

dgozman reviewed Aug 6, 2016
View reviewed changes

src/inspector_agent.cc Outdated

Copy link

Contributor

dgozman Aug 6, 2016

Choose a reason for hiding this comment

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

Let's check that arguments are functions here.

bnoordhuis reviewed Aug 7, 2016
View reviewed changes

lib/internal/bootstrap_node.js Outdated

Copy link

Member

bnoordhuis Aug 7, 2016

Choose a reason for hiding this comment

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

Still a s/sequentally/sequentially/ typo here.

bnoordhuis reviewed Aug 7, 2016
View reviewed changes

src/inspector_agent.cc Outdated

Copy link

Member

bnoordhuis Aug 7, 2016

Choose a reason for hiding this comment

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

Nit: operator should go on previous line.

bnoordhuis reviewed Aug 7, 2016
View reviewed changes

src/inspector_agent.cc Outdated

Copy link

Member

bnoordhuis Aug 7, 2016

Choose a reason for hiding this comment

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

Nit: arguments should line up.

ofrobots reviewed Aug 8, 2016
View reviewed changes

eugeneo mentioned this pull request Aug 8, 2016

node --inspect: dev tools console not working #7891

Closed

trevnorris reviewed Aug 9, 2016
View reviewed changes

jasnell reviewed Aug 9, 2016
View reviewed changes

src/inspector_agent.cc Outdated

Copy link

Member

jasnell Aug 9, 2016

Choose a reason for hiding this comment

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

Would prefer the error message to be a bit more descriptive

ofrobots closed this Aug 12, 2016

cjihrig mentioned this pull request Aug 15, 2016

v6.4.0 Proposal #8070

Merged

MylesBorins added the dont-land-on-v4.x label Sep 9, 2016

Uh oh!

Conversation

alexkozy commented Aug 5, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

alexkozy commented Aug 5, 2016

Uh oh!

jasnell commented Aug 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexkozy commented Aug 5, 2016

Uh oh!

mscdex commented Aug 5, 2016

Uh oh!

alexkozy commented Aug 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexkozy commented Aug 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexkozy commented Aug 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Aug 7, 2016

Uh oh!

alexkozy commented Aug 7, 2016

Uh oh!

jasnell commented Aug 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexkozy commented Aug 8, 2016

Uh oh!

ofrobots commented Aug 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexkozy commented Aug 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Aug 9, 2016

Uh oh!

alexkozy commented Aug 9, 2016

Uh oh!

ofrobots commented Aug 10, 2016

Uh oh!

alexkozy commented Aug 11, 2016

Uh oh!

ofrobots commented Aug 11, 2016

Uh oh!

alexkozy commented Aug 12, 2016

Uh oh!

ofrobots commented Aug 12, 2016

Uh oh!

ofrobots commented Aug 12, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants