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

Support a variablesReference in OutputEvent #79

Closed
hbenl opened this issue Oct 20, 2016 · 12 comments
Closed

Support a variablesReference in OutputEvent #79

hbenl opened this issue Oct 20, 2016 · 12 comments
Assignees
Milestone

Comments

@hbenl
Copy link
Contributor

hbenl commented Oct 20, 2016

In javascript it is possible to send arbitrary objects to the console (e.g. console.log(document.body)). It would be useful if such an object could be inspected in the vscode debug console. Note that this is already possible for evaluation results (via EvaluateResponse.body.variablesReference).

@weinand weinand self-assigned this Nov 2, 2016
@weinand
Copy link
Contributor

weinand commented Nov 2, 2016

/cc @roblourens

@weinand
Copy link
Contributor

weinand commented Nov 17, 2016

@hbenl how would this work for node.js? VS Code does not control the implementation of console.log(<object>). Node's implementation pretty prints the object to stdout (which is captured by the debug adapter as a string and then sent as an OutPutEvent).

@roblourens how would this work in chrome-debug or node-debug2?

@roblourens
Copy link
Member

In chrome-debug, there's an event from the protocol that I listen to for log messages. I could use that in node-debug2, but I just use stdout/stderr at the moment, just because of those messages that are printed to stderr at startup, that aren't captured in the debug protocol.

@weinand
Copy link
Contributor

weinand commented Nov 17, 2016

And this event represents exactly what? Every call to log()? And the event contains the unmodified object passed to log?

@roblourens
Copy link
Member

There's Runtime.consoleAPICalled, and Runtime.exceptionThrown. The first gives the type of console API that was called and its list of arguments, which are in the same format that you get variables or eval results in. So if an object was logged, I can query for its properties on demand.

The second gives an ExceptionDetails object with stacktrace, exception object, etc.

@hbenl
Copy link
Contributor Author

hbenl commented Nov 17, 2016

So it sounds like this would also work for node.js. And Runtime.exceptionThrown sounds like another use case for this feature: uncaught exceptions could be sent to the VS Code debug log and the user could then inspect them there.

@weinand
Copy link
Contributor

weinand commented Nov 18, 2016

More information:

  • CDP API: Runtime.consoleAPICalled. The event passes the console.log arguments as an array.
  • Runtime.consoleAPICalled is not supported in v8 debugger protocol, so node-debug won't be able to support this (without monkey patching console.log).

A straightforward way to support console.log arguments is to add an optional array of variables to the OutputEvent (similar to the VariablesResponse):

body: {
    // ...
    variables: Variable[];
    //...
}

The UI would render only their values without their names (like Chrome dev tools):

2016-11-18_11-30-17

Please note: Currently VS Code is not able to render more than one tree in a row.

Alternatively we could just add an optional attribute variablesReference to the OutputEvent:

body: {
    // ...
    variablesReference?: number;
    //...
}

This would require another VariableRequest to fetch the individual arguments (again as an array of variables).

In this case we could treat the arguments as a single array:

2016-11-18_16-24-48

This would work around the problem with multiple trees in a row.

But we should not make protocol design decisions based on those implementation issues.

Better argument for the second approach are:

  • we never return child items gratuitously without being asked for them via a VariablesRequest
  • passing an array of objects through an OutputEvent seems to leak the specific way how console.log works.

@isidorn @roblourens your opinion?

@roblourens
Copy link
Member

roblourens commented Nov 18, 2016

Hm, I think I would be ok with an array, although in cases like where the it's just logging a bunch of strings, I would probably .join(' ') the in the adapter. It would look nicer if they weren't rendered as an array with the indices, but I don't think it's that much to give up.

On the other hand, it might still be useful beyond console.log, it looks like print in PHP and Python at least accept multiple things to print.

@isidorn
Copy link

isidorn commented Nov 18, 2016

@weinand multiple trees in one row are not currently on the horizon.
Also passing it via an array goes well with our general moto of always fetching variables lazily.

Due to the above I vote for option 2.

Also note that the vscode frontent can workaround option 1 and create a fake parent element because multiiple tree in one row are not supported. But this feels ugly to me.
Also if we introduce multiple trees in a row in the future we could still display them nicely using option 2. Point is both options still leave flexibility to the UI to display data as needed.

@roblourens
Copy link
Member

If we were going to have UI for it in the future, it seems like option 1 would be better so we can tell the difference between console.log(obj1, obj2) and console.log([obj1, obj2]) at some point. I can imagine someone complaining about it. But if it's going to be an ugly hack now or a lot of work, I don't think it's worth it, and option 2 would be fine.

@weinand
Copy link
Contributor

weinand commented Nov 18, 2016

@roblourens in option 2 we can distinguish between log(obj1, obj2) and log([obj1, obj2]) as well: in the former case the VariableRequest returns 2 variables of type object and in the latter case 1 variable of type array. This could be expressed by wrapping the arguments with another array: log([obj1, obj2]) and log([[obj1, obj2]]). The UI would then remove the outer array.

@weinand
Copy link
Contributor

weinand commented Nov 21, 2016

I've added an optional attribute variableReference to the OutputEvent.
@roblourens please consider to use this in the CDP based debug adapters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants