debugger: Fix inconsistent inspector output of `exec new Map()` by cola119 · Pull Request #42423 · nodejs/node · GitHub

Fixed #42405

The output string of exec command is from RemoteObject[customInspectSymbol], but currently it doesn't support some object types such as Map and Set, which leads to invalid string representations (#42405).

This PR implemented string representations when RemoteObject.subtype is set and map. RemoteObject has their abbreviation as ObjectPreview in RemoteObject.preview, which means we can construct their string representation from ObjectPreview.

For example, new Set([ {a: 1}, new Set([1]) ]) is a RemoteObject that has two ObjectPreviews ({a: 1} and new Set([1])).
String representation of ObjectPreview {a: 1} will be {a: 1} and new Set([1]) will be Set(1) { ... }. (Note that ObjectPreview treats an object nested in two or more layers as overflow, so overflowed object is converted to { ... })

1d0da48886f132b362bae680e07cf3b33627a883 added test cases for object type RemoteObject
7820dd4b2542cabb5ea6581be93f5c0d35b7e50a added ObjectPreview and PropertyPreview. And current string representation logics are ported to them.
5a4f767130e50002c98e214382a4dc74ab29f74a fixed string representations of Map and Set.

Thanks for fixing this! I like the new split here, just one nit/question.

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

The way these three commits are organized, the first commit adds a test that fails. This will break git bisect. Can you either move that commit to the end, or else squash the three commits together?

@Trott I squashed them into the one.

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

oy... rather than a long chain of thens... can we convert this into an async function with awaits please?

@jasnell done 3f21ce57a3b5f9047203b1a581a91309ae27e4f5

@Trott @jasnell @jkrems Sorry for bothering you. Could you land this when you are free. Thank you.

I think debugger-* test should be in test/sequential/.

I think debugger-* test should be in test/sequential/.

Most of them are. Maybe this one should be too. The ones in parallel should either not open a port or open a system-provided arbitrary port.

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

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

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

Landed in f890ef5

PR-URL: #42423 Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: nodejs/node#42423 Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

nodejs-github-bot added debugger Issues and PRs related to the debugger subsystem. needs-ci PRs that need a full CI run. labels Mar 21, 2022

cola119 force-pushed the issue/42405 branch from 7e11029 to a69bc81 Compare March 21, 2022 23:51

Trott requested a review from hybrist March 22, 2022 02:41

cola119 force-pushed the issue/42405 branch from a69bc81 to 5a4f767 Compare March 22, 2022 09:48

hybrist approved these changes Mar 23, 2022

View reviewed changes

Comment thread lib/internal/debugger/inspect_repl.js Outdated

cola119 changed the title debugger: Fix Inconsistent inspector output of exec new Map() debugger: Fix inconsistent inspector output of exec new Map() Mar 23, 2022

meixg added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 17, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 17, 2022

cola119 force-pushed the issue/42405 branch from 5a4f767 to a52f32f Compare April 17, 2022 14:26

Trott force-pushed the issue/42405 branch from a52f32f to 82ab123 Compare April 17, 2022 14:31

cola119 force-pushed the issue/42405 branch from 82ab123 to 2f64fcc Compare April 17, 2022 14:31

Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 17, 2022

Trott approved these changes Apr 17, 2022

View reviewed changes

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 17, 2022

github-actions Bot mentioned this pull request Apr 18, 2022

CI Reliability 2022-04-18 nodejs/reliability#255

Open

14 tasks

hybrist approved these changes Apr 18, 2022

View reviewed changes

This was referenced Apr 19, 2022

CI Reliability 2022-04-19 nodejs/reliability#256

Open

CI Reliability 2022-04-20 nodejs/reliability#257

Open

jasnell reviewed Apr 20, 2022

View reviewed changes

This was referenced Apr 21, 2022

CI Reliability 2022-04-21 nodejs/reliability#258

Open

CI Reliability 2022-04-22 nodejs/reliability#259

Open

cola119 force-pushed the issue/42405 branch from 2f64fcc to 3f21ce5 Compare April 22, 2022 11:29

cola119 requested a review from jasnell April 22, 2022 11:30

Trott added the request-ci Add this label to start a Jenkins CI on a PR. label May 7, 2022

cola119 force-pushed the issue/42405 branch from 3f21ce5 to d11e707 Compare May 8, 2022 00:50

cola119 requested a review from Trott May 8, 2022 04:57

Trott approved these changes May 8, 2022

View reviewed changes

Trott added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. labels May 8, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 8, 2022

Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label May 8, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 8, 2022

nodejs-github-bot merged commit f890ef5 into nodejs:master May 8, 2022

RafaelGSS pushed a commit that referenced this pull request May 10, 2022

debugger: fix inconsistent inspector output of exec new Map()

946f57c

PR-URL: #42423 Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

RafaelGSS mentioned this pull request May 10, 2022

v18.2.0 proposal #43036

Merged

github-actions Bot mentioned this pull request May 12, 2022

CI Reliability 2022-05-12 nodejs/reliability#273

Open

30 tasks

joyeecheung mentioned this pull request May 12, 2022

Investigate flaky test-http-server-request-timeout-keepalive #42741

Closed

This was referenced May 12, 2022

CI Reliability 2022-05-12 nodejs/reliability#275

Closed

CI Reliability 2022-05-13 nodejs/reliability#276

Open

CI Reliability 2022-05-14 nodejs/reliability#277

Open

CI Reliability 2022-05-15 nodejs/reliability#278

Open

juanarbol pushed a commit that referenced this pull request May 31, 2022

debugger: fix inconsistent inspector output of exec new Map()

1247809

PR-URL: #42423 Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

juanarbol mentioned this pull request May 31, 2022

2022-05-XX, Version 16.16.0 'Gallium' (LTS) #43268

Closed

danielleadams pushed a commit that referenced this pull request Jun 27, 2022

debugger: fix inconsistent inspector output of exec new Map()

7872da6

PR-URL: #42423 Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

targos pushed a commit that referenced this pull request Jul 12, 2022

debugger: fix inconsistent inspector output of exec new Map()

44c17f2

PR-URL: #42423 Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

targos pushed a commit that referenced this pull request Jul 31, 2022

debugger: fix inconsistent inspector output of exec new Map()

f75a5a9

PR-URL: #42423 Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

targos mentioned this pull request Aug 3, 2022

v16.17.0 release proposal #44098

Merged

guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022

debugger: fix inconsistent inspector output of exec new Map()

f68e526

PR-URL: nodejs/node#42423 Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

debugger: Fix inconsistent inspector output of `exec new Map()` by cola119 · Pull Request #42423 · nodejs/node · GitHub

Uh oh!

Uh oh!

debugger: Fix inconsistent inspector output of exec new Map() - #42423

debugger: Fix inconsistent inspector output of exec new Map()#42423nodejs-github-bot merged 1 commit intonodejs:masternodejs/node:masterfrom cola119:issue/42405cola119/node:issue/42405Copy head branch name to clipboard

Conversation

cola119 commented Mar 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hybrist left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 17, 2022

Uh oh!

Trott commented Apr 17, 2022

Uh oh!

cola119 commented Apr 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 17, 2022

Uh oh!

jasnell Apr 20, 2022

Choose a reason for hiding this comment

Uh oh!

cola119 Apr 22, 2022

Choose a reason for hiding this comment

Uh oh!

cola119 commented May 7, 2022

Uh oh!

cola119 commented May 8, 2022

Uh oh!

Trott commented May 8, 2022

Uh oh!

nodejs-github-bot commented May 8, 2022

Uh oh!

nodejs-github-bot commented May 8, 2022

Uh oh!

nodejs-github-bot commented May 8, 2022

Uh oh!

nodejs-github-bot commented May 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

debugger: Fix inconsistent inspector output of exec new Map()#42423
nodejs-github-bot merged 1 commit into
nodejs:masterfrom
cola119:issue/42405

cola119 commented Mar 21, 2022 •
edited
Loading

cola119 commented Apr 17, 2022 •
edited
Loading