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

[v14.x backport] debugger patches #39446

Closed
wants to merge 24 commits into from
Closed

[v14.x backport] debugger patches #39446

wants to merge 24 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jul 19, 2021

This should help unblock other debugger related backports.

@aduh95 aduh95 requested a review from Trott July 19, 2021 13:48
@github-actions github-actions bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. inspector Issues and PRs related to the V8 inspector protocol needs-ci PRs that need a full CI run. v14.x labels Jul 19, 2021
Trott
Trott approved these changes Jul 19, 2021
View reviewed changes
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.

LGTM with line-wrap lint fixes and a green CI

@aduh95 aduh95 mentioned this pull request Jul 19, 2021
Closed
@richardlau
Copy link
Member

richardlau commented Jul 19, 2021 •
edited by aduh95
Loading

AFAICT this backports the following (listing so they're linked):

This was referenced Jul 19, 2021
Merged
Closed
Merged
Merged
Merged
Merged
Closed
Merged
Merged
@richardlau

This comment has been minimized.

Sign in to view
@aduh95 aduh95 force-pushed the backport-debugger-patches branch from 224c3d8 to f5ec086 Compare July 20, 2021 12:14
aduh95 and others added 10 commits July 20, 2021 15:18
@aduh95
lib: add primordials.SafeArrayIterator
c53b4ad 
PR-URL: nodejs#36532
Reviewed-By: Rich Trott <rtrott@gmail.com>
@aduh95
Revert "debugger: rename internal library for clarity"
1355e52 
This reverts commit d0c7ab0.
@RafaelGSS @aduh95
debugger: add usage example for --port
7b5e17f 
PR-URL: nodejs#38400
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@aduh95
debugger: refactor internal/inspector/_inspect to use more primordials
8146a8c 
PR-URL: nodejs#38406
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos @aduh95
debugger: wait for V8 debugger to be enabled
cc589f3 
Refs: nodejs#38273 (comment)

PR-URL: nodejs#38811
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott @aduh95
debugger: revise async iterator usage to comply with lint rules
e036082 
I'm not sure that this is any clearer than the existing code, but I
don't think it's significantly less clear, and it avoids comment
disabling a lint rule.

PR-URL: nodejs#38847
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott @aduh95
debugger: reduce scope of eslint disable comment
e622105 
Current code masks setInterval and setTimeout with promisified versions.
This can be confusing to read and causes lint errors. Replace masking
with use of pSetInterval and pSetTimeout instead.

Move disabling of lint rule from entire file to the one remaining line
(after the above changes) that still needs it.

PR-URL: nodejs#38946
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@aduh95
debugger: enable linter on internal/inspector/inspect_client
9b960d9 
PR-URL: nodejs#38417
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott @aduh95
debugger: remove unnecessary boilerplate copyright comment
5ae4a50 
PR-URL: nodejs#38952
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@Trott @aduh95
debugger: align message with Node.js standard
ccc69e0 
Node.js uses (or will use) _Ctrl+C_ with no spaces. To unify the
messages from inspect with other messages from the REPL and elsewhere,
make that change in node-inspect too.

PR-URL: nodejs#38400
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
richardlau pushed a commit that referenced this pull request Jul 22, 2021
@Trott @richardlau
errors: add ERR_DEBUGGER_ERROR
cfccf13 
PR-URL: #39024
Backport-PR-URL: #39446
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
richardlau pushed a commit that referenced this pull request Jul 22, 2021
@Trott @richardlau
debugger: use ERR_DEBUGGER_ERROR in debugger client
a3991d7 
PR-URL: #39024
Backport-PR-URL: #39446
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
richardlau pushed a commit that referenced this pull request Jul 22, 2021
@Trott @richardlau
debugger: use error codes in debugger REPL
5d4d23d 
PR-URL: #39024
Backport-PR-URL: #39446
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
richardlau pushed a commit that referenced this pull request Jul 22, 2021
@Trott @richardlau
errors: add ERR_DEBUGGER_STARTUP_ERROR
9b993ed 
PR-URL: #39024
Backport-PR-URL: #39446
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
richardlau pushed a commit that referenced this pull request Jul 22, 2021
@Trott @richardlau
debugger: use ERR_DEBUGGER_STARTUP_ERROR in _inspect.js
b7ee9d8 
PR-URL: #39024
Backport-PR-URL: #39446
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
richardlau pushed a commit that referenced this pull request Jul 22, 2021
@Trott @richardlau
debugger: rename internal library for clarity
716ee15 
When I moved these files from node-inspect to Node.js core, I put them
in lib/internal/inspector. That was a mistake. They should be in
lib/internal/debugger.

PR-URL: #39080
Backport-PR-URL: #39446
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@richardlau richardlau mentioned this pull request Jul 22, 2021
Merged
@aduh95 aduh95 deleted the backport-debugger-patches branch July 22, 2021 17:06
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@aduh95 @foxxyz
lib: add primordials.SafeArrayIterator
9d57349 
PR-URL: nodejs#36532
Backport-PR-URL: nodejs#39446
Reviewed-By: Rich Trott <rtrott@gmail.com>
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@aduh95 @foxxyz
Revert "debugger: rename internal library for clarity"
b25548d 
This reverts commit d0c7ab0.

PR-URL: nodejs#39446
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@RafaelGSS @foxxyz
debugger: add usage example for --port
8a789a4 
PR-URL: nodejs#38400
Backport-PR-URL: nodejs#39446
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@aduh95 @foxxyz
debugger: refactor internal/inspector/_inspect to use more primordials
001baa0 
PR-URL: nodejs#38406
Backport-PR-URL: nodejs#39446
Reviewed-By: Rich Trott <rtrott@gmail.com>
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@targos @foxxyz
debugger: wait for V8 debugger to be enabled
f9c6beb 
Refs: nodejs#38273 (comment)

PR-URL: nodejs#38811
Backport-PR-URL: nodejs#39446
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@Trott @foxxyz
debugger: revise async iterator usage to comply with lint rules
5998510 
I'm not sure that this is any clearer than the existing code, but I
don't think it's significantly less clear, and it avoids comment
disabling a lint rule.

PR-URL: nodejs#38847
Backport-PR-URL: nodejs#39446
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@Trott @foxxyz
debugger: reduce scope of eslint disable comment
8ad9cff 
Current code masks setInterval and setTimeout with promisified versions.
This can be confusing to read and causes lint errors. Replace masking
with use of pSetInterval and pSetTimeout instead.

Move disabling of lint rule from entire file to the one remaining line
(after the above changes) that still needs it.

PR-URL: nodejs#38946
Backport-PR-URL: nodejs#39446
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@aduh95 @foxxyz
debugger: enable linter on internal/inspector/inspect_client
9b0d4a1 
PR-URL: nodejs#38417
Backport-PR-URL: nodejs#39446
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@Trott @foxxyz
debugger: remove unnecessary boilerplate copyright comment
754989e 
PR-URL: nodejs#38952
Backport-PR-URL: nodejs#39446
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@Trott @foxxyz
debugger: align message with Node.js standard
20fea80 
Node.js uses (or will use) _Ctrl+C_ with no spaces. To unify the
messages from inspect with other messages from the REPL and elsewhere,
make that change in node-inspect too.

PR-URL: nodejs#38400
Backport-PR-URL: nodejs#39446
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@Trott @foxxyz
debugger: wrap lines longer than 80 chars
105043e 
Wrap lines more than 80 chararcters long in inspect_repl.js so we can
disable specific rules.

PR-URL: nodejs#38529
Backport-PR-URL: nodejs#39446
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@Trott @foxxyz
debugger: avoid non-ASCII char in code file
1081059 
Avoid non-ASCII char in lib code as a single non-ASCII char forces all
the chars to be stored inefficiently, bloating the binary size. This
also brings the file closer to compatibility with our lint rules.

PR-URL: nodejs#38529
Backport-PR-URL: nodejs#39446
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@Trott @foxxyz
debugger: disable only the lint rules required by current file state
e706b8d 
PR-URL: nodejs#38529
Backport-PR-URL: nodejs#39446
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@aduh95 @foxxyz
debugger: refactor to use internal modules
7fa1f73 
This avoids loading the entirety of `node:util` and `node:url` and their
dependencies while only a subset is actually used by this module.

PR-URL: nodejs#38550
Backport-PR-URL: nodejs#39446
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@aduh95 @foxxyz
debugger: refactor inspect_repl to use primordials
c718162 
PR-URL: nodejs#38551
Backport-PR-URL: nodejs#39446
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@Trott @foxxyz
debugger: removed unused function argument
29757af 
PR-URL: nodejs#38850
Backport-PR-URL: nodejs#39446
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@Trott @foxxyz
errors: add ERR_DEBUGGER_ERROR
af9e841 
PR-URL: nodejs#39024
Backport-PR-URL: nodejs#39446
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@Trott @foxxyz
debugger: use ERR_DEBUGGER_ERROR in debugger client
753e2be 
PR-URL: nodejs#39024
Backport-PR-URL: nodejs#39446
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@Trott @foxxyz
debugger: use error codes in debugger REPL
3b1ab39 
PR-URL: nodejs#39024
Backport-PR-URL: nodejs#39446
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@Trott @foxxyz
errors: add ERR_DEBUGGER_STARTUP_ERROR
bb38c19 
PR-URL: nodejs#39024
Backport-PR-URL: nodejs#39446
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@Trott @foxxyz
debugger: use ERR_DEBUGGER_STARTUP_ERROR in _inspect.js
99cb640 
PR-URL: nodejs#39024
Backport-PR-URL: nodejs#39446
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
foxxyz pushed a commit to foxxyz/node that referenced this pull request Oct 18, 2021
@Trott @foxxyz
debugger: rename internal library for clarity
0efdeea 
When I moved these files from node-inspect to Node.js core, I put them
in lib/internal/inspector. That was a mistake. They should be in
lib/internal/debugger.

PR-URL: nodejs#39080
Backport-PR-URL: nodejs#39446
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott approved these changes

@richardlau richardlau richardlau approved these changes

Assignees
No one assigned
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. inspector Issues and PRs related to the V8 inspector protocol 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
@aduh95 @richardlau @nodejs-github-bot @Trott @RafaelGSS @targos