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

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

Merged
merged 1 commit into from
May 8, 2022

Conversation

cola119
Copy link
Member

@cola119 cola119 commented Mar 21, 2022

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 { ... })

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

@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 Mar 21, 2022
@Trott Trott requested a review from jkrems March 22, 2022 02:41
Copy link
Contributor

@jkrems jkrems left a comment

Choose a reason for hiding this comment

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

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

lib/internal/debugger/inspect_repl.js Show resolved Hide resolved
@cola119 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 meixg added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 17, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 17, 2022
@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Apr 17, 2022

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?

@cola119
Copy link
Member Author

cola119 commented Apr 17, 2022

@Trott I squashed them into the one.

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

throw error;
}

cli.waitForInitialBreak()
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@cola119
Copy link
Member Author

cola119 commented May 7, 2022

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

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label May 7, 2022
@cola119
Copy link
Member Author

cola119 commented May 8, 2022

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

@Trott
Copy link
Member

Trott commented May 8, 2022

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.

@cola119 cola119 requested a review from Trott May 8, 2022 04:57
@Trott 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 pending requests for changes, and a CI started. labels May 8, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 8, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@Trott Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label May 8, 2022
@nodejs-github-bot 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 nodejs-github-bot merged commit f890ef5 into nodejs:master May 8, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in f890ef5

RafaelGSS pushed a commit that referenced this pull request May 10, 2022
PR-URL: #42423
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request May 10, 2022
juanarbol pushed a commit that referenced this pull request May 31, 2022
PR-URL: #42423
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
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
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
PR-URL: #42423
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
PR-URL: nodejs/node#42423
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. debugger Issues and PRs related to the debugger subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent inspector output of exec new Map()
6 participants