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

Fix application console.log output #168

Merged
merged 19 commits into from
May 22, 2024

Conversation

filip131311
Copy link
Collaborator

@filip131311 filip131311 commented Apr 26, 2024

This PR fixes a problem with simulated application logs not outputting objects correctly.

Fixes: #161

This PR adds indenting for objects and treats every console.log argument as a filed (consistently with how native vscode debugConsole works). To achieve this a "virtual" root object is created and a variablesRefrence to this object is passed into OutputEvent.

Before:

Screen.Recording.2024-04-27.at.00.24.47.mov

After:

Screen.Recording.2024-05-14.at.02.08.01.mov

Copy link

vercel bot commented Apr 26, 2024

Someone is attempting to deploy this pull request to the software-mansion Team on Vercel.

To accomplish this, the commit author's email address needs to be associated with a GitHub account.

Learn more about how to change the commit author information.

@filip131311 filip131311 changed the title Fix application console.log output. (WIP) Fix application console.log output. Apr 29, 2024
@filip131311 filip131311 changed the title (WIP) Fix application console.log output. Fix application console.log output. May 8, 2024
depth: number,
prefix?: string
): Promise<FormmatedLog> {
if (depth > MAX_OBJECT_DEPTH) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This prevents infinite call.

@filip131311 filip131311 requested a review from kmagiera May 8, 2024 23:26
Copy link
Member

@jakub-gonet jakub-gonet left a comment

Choose a reason for hiding this comment

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

I don't quite get the method used to print here. I'd expect that we'd need only label and list of subnodes?

@@ -30,6 +32,16 @@ export type CDPDebuggerScope = {
object: CDPRemoteObject & { type: "object" };
};

export type FormmatedLog = {
Copy link
Member

Choose a reason for hiding this comment

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

Typo: replace FormmatedLog with FormatedLog

Copy link
Member

Choose a reason for hiding this comment

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

❓ What does prefix, indented, and unindented do?

Comment on lines 174 to 175
// For now this is a neccesery workaround, because of a bug in vs code that prevents "source" and "group" properties to work together
// TODO: monitore if the bug was solved and remove Source Event https://github.com/microsoft/vscode/issues/212304
Copy link
Member

Choose a reason for hiding this comment

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

Typo in "neccesery" and "monitore".

Copy link
Member

Choose a reason for hiding this comment

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

By the way, the comment doesn't explain why the workaround is here. I don't understand why it's even needed in the first place.

sourceEvent.body = {
...sourceEvent.body,
category: log.category,
//@ts-ignore source, line, column and group are valid fieleds
Copy link
Member

Choose a reason for hiding this comment

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

Typo: "fieleds".

};
this.sendEvent(startCollapsedEvent);

log.indented.map((item) => {
Copy link
Member

Choose a reason for hiding this comment

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

This should be forEach or even better, for-of loop.

packages/vscode-extension/src/debugging/DebugAdapter.ts Outdated Show resolved Hide resolved
Comment on lines 160 to 164
.map((item) => {
return item?.unindented;
})
.join(" ")
.slice(0, 79);
Copy link
Member

Choose a reason for hiding this comment

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

  1. 80 line length should be extracted to some constant.
  2. This probably isn't a bottleneck but joining all things just to drop everything after 80 characters feels wasteful.
  3. nit:
.map((item) => {
       return item?.unindented;
     })

// same as
.map(({unindented}) => unindented)

break;
case "function":
res.indented.push({
unindented: prop.name + ": " + (prop.description || function () {}),
Copy link
Member

Choose a reason for hiding this comment

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

Why we're converting function to string as fallback to description?

});
return format(obj);
const res = {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: use full words.

@jakub-gonet jakub-gonet changed the title Fix application console.log output. Fix application console.log output May 8, 2024
Copy link
Member

@kmagiera kmagiera left a comment

Choose a reason for hiding this comment

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

I think that we should use the same approach vscode-js-debug uses. Apparently debugadapter module we use from NPM doesn't have all the properties of OutpuEvent, but both spec and vscode-js-debug have variableReference prop which I think we should be using here instead:

https://microsoft.github.io/debug-adapter-protocol/specification#Events_Output

https://github.com/microsoft/vscode-js-debug/blob/main/src/dap/api.d.ts#L2872

@filip131311 filip131311 requested a review from kmagiera May 14, 2024 00:11
Copy link

vercel bot commented May 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-native-ide ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 20, 2024 4:16pm

Copy link
Member

@kmagiera kmagiera left a comment

Choose a reason for hiding this comment

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

Feels like this has improved a lot since original version but likely some improvements here are going to be needed. Lets merge it as is and improve in follow up

@kmagiera kmagiera merged commit 0db852c into main May 22, 2024
1 of 2 checks passed
@kmagiera kmagiera deleted the @Filip131311/FixAplicationConsoleLogs branch May 22, 2024 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

debug console logs don't include full message when including objects or other structures
3 participants