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.

Sign up for GitHub

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

Jump to bottom

debugger: refactor console in lib/internal/debugger/inspect.js #45847

Merged
merged 3 commits into from
Dec 28, 2022
Merged

debugger: refactor console in lib/internal/debugger/inspect.js #45847

merged 3 commits into from
Dec 28, 2022

Conversation

debadree25
Copy link
Member

@debadree25 debadree25 commented Dec 13, 2022 •
edited
Loading

Came across TODO by @aduh95 in https://github.com/nodejs/node/blob/0665fa4009e2aa26adb95c27a363cc7ff0686d41/lib/internal/debugger/inspect.js to remove usage of console in the file, have attempted to do that by replacing console with process.stderr.write().

Refs: #38406

@nodejs-github-bot nodejs-github-bot added debugger Issues and PRs related to the debugger subsystem. needs-ci PRs that need a full CI run. labels Dec 13, 2022
aduh95
aduh95 approved these changes Dec 14, 2022
View reviewed changes
lib/internal/debugger/inspect.js Outdated Show resolved Hide resolved
lib/internal/debugger/inspect.js Outdated Show resolved Hide resolved
debadree25 and others added 2 commits December 14, 2022 11:54
@debadree25 @aduh95
update message to use template string
1e4f985 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@debadree25 @aduh95
update message to use template string
9c08a5b 
Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>
@RaisinTen
Copy link
Contributor

What's the reason for this TODO btw?

@debadree25
Copy link
Member Author

debadree25 commented Dec 14, 2022 •
edited
Loading

What's the reason for this TODO btw?

I guess it plays along with the original PRs intention of refactoring to use more primordials theme?

@RaisinTen
Copy link
Contributor

But process and console are not primordials though

@debadree25
Copy link
Member Author

But process and console are not primordials though

Ah my bad 😅😅😅

@RaisinTen
Copy link
Contributor

:-)

Maybe @aduh95 knows?

joyeecheung
joyeecheung approved these changes Dec 16, 2022
View reviewed changes
lib/internal/debugger/inspect.js
console.error('There was an internal error in Node.js. ' +
'Please report this bug.');
console.error(e.message);
console.error(e.stack);
Copy link
Member

Choose a reason for hiding this comment

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

There would be a difference if the inspect client listens to Runtime.consoleAPICalled, but I don't think it does currently.

@cola119 cola119 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Dec 19, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 19, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Dec 20, 2022
Open
14 tasks
@cola119 cola119 added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 20, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 20, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@cola119 cola119 added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 20, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 20, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Dec 21, 2022
Open
12 tasks
@cola119 cola119 added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 21, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 21, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Dec 22, 2022
Open
14 tasks
@cola119 cola119 added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 24, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 24, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Dec 25, 2022
Open
8 tasks
@cola119 cola119 added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 25, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 25, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@cola119 cola119 added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Dec 25, 2022
This was referenced Dec 26, 2022
Open
Open
Open
@aduh95
Copy link
Contributor

aduh95 commented Dec 28, 2022

:-)

Maybe @aduh95 knows?

TBH I don't have any recollection of adding this TODO, so I also have to guess what my past self was thinking at the time. Using WritableStream.prototype.write instead of console.log does reduce the surface of exposed internal APIs used here, so that seems like a good step towards a more robust debugger.

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 28, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 28, 2022
@nodejs-github-bot nodejs-github-bot merged commit 17cb655 into nodejs:main Dec 28, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 17cb655

vitpavlenko pushed a commit to vitpavlenko/node that referenced this pull request Dec 28, 2022
@debadree25 @vitpavlenko
debugger: refactor console in lib/internal/debugger/inspect.js
a91b2eb 
Refs: nodejs#38406
PR-URL: nodejs#45847
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>
This was referenced Dec 29, 2022
Open
Open
Open
Open
targos pushed a commit that referenced this pull request Jan 1, 2023
@debadree25 @targos
debugger: refactor console in lib/internal/debugger/inspect.js
e2594ac 
Refs: #38406
PR-URL: #45847
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>
@github-actions github-actions bot mentioned this pull request Jan 2, 2023
Open
22 tasks
@RafaelGSS RafaelGSS mentioned this pull request Jan 2, 2023
Merged
@github-actions github-actions bot mentioned this pull request Jan 3, 2023
Open
23 tasks
RafaelGSS pushed a commit that referenced this pull request Jan 4, 2023
@debadree25 @RafaelGSS
debugger: refactor console in lib/internal/debugger/inspect.js
f1e300c 
Refs: #38406
PR-URL: #45847
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Jan 5, 2023
@debadree25 @RafaelGSS
debugger: refactor console in lib/internal/debugger/inspect.js
3d09754 
Refs: #38406
PR-URL: #45847
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
@debadree25 @juanarbol
debugger: refactor console in lib/internal/debugger/inspect.js
34f3912 
Refs: #38406
PR-URL: #45847
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>
@juanarbol juanarbol mentioned this pull request Jan 28, 2023
Merged
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
@debadree25 @juanarbol
debugger: refactor console in lib/internal/debugger/inspect.js
4cdf000 
Refs: #38406
PR-URL: #45847
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cola119 cola119 cola119 approved these changes

@aduh95 aduh95 aduh95 approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. debugger Issues and PRs related to the debugger subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@debadree25 @RaisinTen @nodejs-github-bot @aduh95 @joyeecheung @cola119