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

doc: add description for inspector-only console methods. #17004

Closed
wants to merge 12 commits into from

Conversation

Tiriel
Copy link
Contributor

@Tiriel Tiriel commented Nov 13, 2017

Description inspired by dev tools reference and inspector err messages

Added:

  • intro
  • console.debug()
  • console.dirxml()
  • console.markTimeline()
  • console.profile()
  • console.profileEnd()
  • console.table()
  • console.timeStamp()
  • console.timeline()
  • console.timelineEnd()

Fixes: #16755

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

doc

@nodejs-github-bot nodejs-github-bot added console Issues and PRs related to the console subsystem. doc Issues and PRs related to the documentations. labels Nov 13, 2017
@@ -417,10 +417,101 @@ added: v0.1.100

The `console.warn()` function is an alias for [`console.error()`][].

## Inspector only methods
The following methods are exposed by the V8 engine in the general API but are non-op unless used
Copy link
Member

Choose a reason for hiding this comment

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

I'd spell out what you mean by non-op rather than trust that readers will know what that means. So instead of are non-op, perhaps do not display anything or something like that.

@Trott
Copy link
Member

Trott commented Nov 14, 2017

Would it be better for users if we repeat the "these functions don't do anything if you're not in the inspector" warning for each method rather than trusting that users will see the notice at the top? I know I often only read the text that is directly under the function that I care about in the docs.

@Tiriel Tiriel force-pushed the doc-add-console-noops branch from 8f59344 to 7ca8aa7 Compare November 14, 2017 12:51
@Tiriel
Copy link
Contributor Author

Tiriel commented Nov 14, 2017

@Trott Thanks for the input! Just did it, hope it's okay


This method does not display anything unless used in the inspector. The
`console.profile()` method starts a JavaScript CPU profile with an optional
label until [`console.profileEnd()`][] is called. The profile is then added to
Copy link
Contributor

Choose a reason for hiding this comment

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

A nit: undefined reference [`console.profileEnd()`][]

This method does not display anything unless used in the inspector. Stops the
current JavaScript CPU profiling session if one has been started and prints
the report to the **Profiles** panel of the inspector tab. See
[`console.profile()`][] for an example.
Copy link
Contributor

Choose a reason for hiding this comment

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

A nit: undefined reference [`console.profile()`][]

<!-- YAML
added: v8.0.0
-->
* `array` {array|object}
Copy link
Contributor

Choose a reason for hiding this comment

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

A nit: non-primitive types are usually noted in title case: {Array|Object}

[`console.error()`]: #console_console_error_data_args
[`console.group()`]: #console_console_group_label
[`console.log()`]: #console_console_log_data_args
[`console.time()`]: #console_console_time_label
[`console.timeStamp()`]: #console_console_timestamp_label
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Nov 14, 2017

Choose a reason for hiding this comment

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

A nit: this reference should go after the [`console.timeEnd()`], ABC-wise.

@Tiriel Tiriel force-pushed the doc-add-console-noops branch from 7ca8aa7 to 02a3493 Compare November 14, 2017 14:22
@@ -417,11 +417,114 @@ added: v0.1.100

The `console.warn()` function is an alias for [`console.error()`][].

## Inspector only methods
The following methods are exposed by the V8 engine in the general API but do
not display anything unless used in conjunction with the inspector tab
Copy link
Member

Choose a reason for hiding this comment

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

having inspector be a link to debugger.html and/or inspector.html would be good here. debugger.html is the best choice.

@Tiriel Tiriel force-pushed the doc-add-console-noops branch from 02a3493 to 99e297d Compare November 14, 2017 19:23
@Tiriel
Copy link
Contributor Author

Tiriel commented Nov 14, 2017

Thanks! Changed according to each review :)

@@ -417,15 +417,119 @@ added: v0.1.100

The `console.warn()` function is an alias for [`console.error()`][].

## Inspector only methods
The following methods are exposed by the V8 engine in the general API but do
not display anything unless used in conjunction with the [inspector][] tab
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Remove tab.

* `data` {any}
* `...args` {any}

The `console.debug()` function is an alias for [`console.log()`][].
Copy link
Member

Choose a reason for hiding this comment

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

Nit: This also needs the "does not display anything" line. Although I guess we could easily implement console.debug() if it's just an alias to console.log()...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I can't say for sure, but that is what I saw in the inspector and what seems to indicate the DevTools ref: https://developers.google.com/web/tools/chrome-devtools/console/console-reference#consoledebugobject_object

Wouldn't it be better then if I leave this line like this and open another issue/PR to implement the method?

@Trott
Copy link
Member

Trott commented Nov 14, 2017

@Tiriel Thanks for your patience with all the nits. Doc updates can be like that.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Would like to see the recent round of nits addressed, but LGTM with or without them.

@Tiriel Tiriel force-pushed the doc-add-console-noops branch from 99e297d to bcfe296 Compare November 14, 2017 21:42
@Tiriel
Copy link
Contributor Author

Tiriel commented Nov 14, 2017

@Trott No problem at all, having correct api docs seems of vital importance to me. That's the source of truth for many users/devs, they need to be the best we can provide.

Removed the 'tab's!

@tniessen
Copy link
Member

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

@Tiriel
Copy link
Contributor Author

Tiriel commented Nov 20, 2017

Awesome, thanks!

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 21, 2017
jasnell pushed a commit that referenced this pull request Nov 22, 2017
Description inspired by dev tools reference and inspector err messages

Added:
* intro
* console.debug()
* console.dirxml()
* console.markTimeline()
* console.profile()
* console.profileEnd()
* console.table()
* console.timeStamp()
* console.timeline()
* console.timelineEnd()

PR-URL: #17004
Fixes: #16755
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell
Copy link
Member

jasnell commented Nov 22, 2017

Landed in 982c674

@jasnell jasnell closed this Nov 22, 2017
@Tiriel
Copy link
Contributor Author

Tiriel commented Nov 22, 2017

Thanks!

@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 28, 2017
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
Adds the console.debug() method, alias for console.log(). This method is
exposed by V8 and was only available in inspector until now. Also adds
matching test and documentation.

PR-URL: #17033
Refs: #17004
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
Description inspired by dev tools reference and inspector err messages

Added:
* intro
* console.debug()
* console.dirxml()
* console.markTimeline()
* console.profile()
* console.profileEnd()
* console.table()
* console.timeStamp()
* console.timeline()
* console.timelineEnd()

PR-URL: #17004
Fixes: #16755
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
Description inspired by dev tools reference and inspector err messages

Added:
* intro
* console.debug()
* console.dirxml()
* console.markTimeline()
* console.profile()
* console.profileEnd()
* console.table()
* console.timeStamp()
* console.timeline()
* console.timelineEnd()

PR-URL: #17004
Fixes: #16755
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
MylesBorins pushed a commit that referenced this pull request Jan 15, 2018
Adds the console.debug() method, alias for console.log(). This method is
exposed by V8 and was only available in inspector until now. Also adds
matching test and documentation.

PR-URL: #17033
Refs: #17004
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member

Trott commented Dec 8, 2018

Turns out this should have landed in v8.x. See #24909 (comment).

Miraculously, 982c674 still cherry-picks cleanly to the v8.x-staging branch. I've removed the dont-land-on-v8.x label. Is that enough for it to get cherry-picked into the next v8.x release?

(Pinging the last three people who did v8.x releases for an answer to that last question: @rvagg @BethGriggs @MylesBorins.)

MylesBorins pushed a commit that referenced this pull request Dec 10, 2018
Description inspired by dev tools reference and inspector err messages

Added:
* intro
* console.debug()
* console.dirxml()
* console.markTimeline()
* console.profile()
* console.profileEnd()
* console.table()
* console.timeStamp()
* console.timeline()
* console.timelineEnd()

PR-URL: #17004
Fixes: #16755
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins
Copy link
Contributor

I've landed this in 8.x-staging

@BethGriggs can you rebase this into the current release proposal

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
console Issues and PRs related to the console subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.