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

util: make debuglog show up in --inspect logs #41884

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Feb 7, 2022

This makes util.debuglog() show up in the inspector console under the debug level.

@joyeecheung I've removed the TODO comment since it loses data prior to connection of an inspector which is a UX degradation from the existing situation and doesn't match other environments for JS.

This does cause the same kind of memory leak while retaining messages as console currently does but it only does so if the specific debuglog set is enabled.

@bmeck bmeck added util Issues and PRs related to the built-in util module. inspector Issues and PRs related to the V8 inspector protocol labels Feb 7, 2022
@bmeck bmeck requested a review from joyeecheung February 7, 2022 16:16
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 7, 2022

Review requested:

  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Feb 7, 2022
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.

Removing the TODO LGTM (it already doesn't matter now that we snapshot the console methods) , but I have a question regarding the implementation of the wrapping

lib/internal/util/debuglog.js Outdated Show resolved Hide resolved
@bmeck bmeck force-pushed the debuglog-inspector branch from af442bd to fdb31dd Compare March 7, 2022 21:21
@bmeck
Copy link
Member Author

bmeck commented Mar 7, 2022

To fix some tests I had to make colors work, this causes some dependency on #32308 in a way. I think I addressed the concern in that PR though by injecting the color setting via DI rather than doing it in all Console objects.

CC: @addaleax @BridgeAR

@bmeck bmeck added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 7, 2022
@bmeck bmeck requested review from addaleax and BridgeAR March 7, 2022 21:23
lib/internal/console/constructor.js Outdated Show resolved Hide resolved
lib/internal/console/global.js Outdated Show resolved Hide resolved
test/embedding/test-embedding.js Outdated Show resolved Hide resolved
lib/internal/console/global.js Outdated Show resolved Hide resolved
@bmeck
Copy link
Member Author

bmeck commented Mar 8, 2022

@addaleax I've pulled out the console changes for now. Would it be fine to have these as a separate PR still?

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Would it be fine to have these as a separate PR still?

I think so -- it's definitely a breaking change, though. (This PR doesn't need to be semver-major anymore imo.)

@bmeck bmeck removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 9, 2022
@nodejs-github-bot
Copy link
Collaborator

@antsmartian
Copy link
Contributor

@bmeck Ping. Are we planning to get this to a closure?

@aduh95
Copy link
Contributor

aduh95 commented May 11, 2024

This needs a rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inspector Issues and PRs related to the V8 inspector protocol needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants