-
Notifications
You must be signed in to change notification settings - Fork 96
Treat Breakpoint logMessage as expression #174
Conversation
Change LogPoints's logMessage to be evaluated as an expression and not a string.
Thanks for the PR, but expression interpolation is a bit more tricky. The log message is plain text (as in VS and VS for Mac), not an expression. Expressions are only interpolated within curly brackets. node-debug's implementation will be in the next Insiders. |
What's the syntax for interruptions @weinand? String literals? The challenge is that we want to provide a better console.log, and developers will probably expect to be able to pass multiple arguments as in |
syntax is identical to VS and VS for Mac:
Inside curly braces the language of the underlying source can be used (and soon Intellisense). |
I'm not sure this is a great experience for JavaScript developers. We shouldn't introduce a new custom syntax for LogPoints if the store telling and positioning is "a better console.log". JavaScript devs won't care about a VS or VS for Mac syntax, but how it feels in JavaScript. If we think string interpolation is the way to go, we should at least use https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals |
To support "a better console.log" story for all debuggers, we should not cling to the idiosyncrasies of JavaScript's console.log by itself supports C-style
Then there is the somewhat laborious way of "adding" strings:
And last but not least the "Template literals":
And here again the VS syntax (which differs from "Template literals" only by the dropped '$' and backticks):
For me it's pretty obvious what syntax is most appealing and readable and easy to type. And it does not not make a lot of sense to drop VS compatibility by introducing a '$'. In addition, I have to wear my multi-language-debugging hat. And with that hat on I prefer the somewhat unbiased VS syntax. @auchenberg What is your concrete proposal? |
My specific proposal is that we should enable language specific syntaxes for LogPoint messages, instead of introducing a VS/VSCode specific and language neutral for a new debugging feature. We should follow the language specific standards for string interpolation, and the standards for how the usual logging helper mechanism works. We should meet developers there they are instead of giving them a new syntax to learn. C#:
(I assume this is where the existing VS syntax is coming from) Ruby:
Go:
JavaScript: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals
Keeping our LogPoint messages compatible with the used language can also enable us to convert LogPoints to persisted log message by injecting |
You said:
But then you are proposing just another new syntax: your approach is basically a "crippled syntax" approach: remove the name of the language specific log function (the "context") and only keep the rest. Is this really preferable over VS's minimalistic approach? e.g.
vs
For your approach it will be very difficult to provide Intellisense because for every language, VS code would need to know what syntactic construct the logMessage represents. But this approach is not only difficult to support in VS Code but it is also problematic for devs that work with different languages. They always have to understand how the entered logMessage maps to the underlying implementation otherwise it will be difficult to understand what to enter in Go vs. JavaScript. Your last argument:
holds equally well (or even better) for the proposed VS syntax: In the VS syntax it is well defined that the logMessage is text and that curly braces contain language specific expressions. Converting that into a different logging method at a later point is always possible. Doing the same with logMessages |
Let's get additional opinions from some of the DAP stakeholders: For context: in this milestone we are introducing "logPoint" support by adding a string-valued Now the question is: what is the semantics (and syntax) for
|
No, I'm not proposing to introduce a new syntax as JavaScript developers already are using Template Literals and multiple arguments with I don't consider existing users of VS/VS for Mac as the target audience for LogPoints in VS Code, and the VS syntax seem to be a result of them shipping LogPoints for C# first. |
For C# I would imagine we would want to use the existing VS syntax, since most C# developers are familiar with Visual Studio. There are a few problems that get harder (but certainly not impossible) if one tries to use a different tracing syntax for each language:
The only other problem I can think of with using a language-specific syntax is that in some languages (ex: C++, sounds like Go as well) the language specific syntax is definitely harder to use than the language neutral syntax. Though clearly in these cases one could just choose the language neutral syntax instead. |
If we use the common syntax with braces indicating expressions, would the protocol be changed so that this is enforced on vscode's end? Because otherwise we're just debating how node-debug should handle it, and there's no guarantee that any other debug adapter would make the same decision. And you can't really provide intellisense for it without an agreement between the language service and debug adapter on the syntax. |
In my opinion, the main advantage of doing it the "VS way" is that it can be implemented entirely at the UI layer. VS doesn't expose anything about tracepoints to the debugger - it treats them as normal breakpoints. When a breakpoint is hit, VS recognizes that it has a tracepoint string associated with it, splits out everything between curly-brackets, asks the debugger to evaluate those strings, inserts the results back into the original string, prints it, and tells the debugger to resume execution. A solution like this will "just work" for any debugger, without requiring any work on the debugger side. |
I think we allow the debug adapter to implement it because the Node adapter can have the runtime handle everything, much faster than making round trips. Especially important for attaching to production apps, which is a main use case. We could actually have it both ways - implement it on the client for all types, unless the adapter opts in to handling it for itself. |
@auchenberg I said that your approach introduces "new syntax" because it is not very common that a text box expects a specific argument list format (without the corresponding function) instead of a plain vanilla expression or a statement. How should a dev know that for JavaScript we are supporting only template-syntax but not the "classic" console.log format with embedded "%s". @roblourens the DAP spec says for the
As usual there is nothing node.js specific about this. So this syntax applies to all DAs that opt into this via the corresponding capability. VS Code will enforce the syntax, but it will stop at the curly braces. But we are already working on supporting language specific Intellisense within the braces. The language is determined by the underlying editor of the logPoint. Within a curly brace we assume expression syntax, the same that we will support for watch expressions or Breakpoint "conditions". If we would have to support language specific argument lists, we would have to know the declaration of the underlying log-function. But the underlying log-function is DA specific so it will be difficult for VS Code to get this information (especially if the debug extension is not active). @andrewcrawley your comment is basically what we are doing in VS Code and what motivated us to use that approach. |
For the past hours I've been demoing LogPoints here at TechFest, and there's a few good learnings. I think this screenshot from a demo is a good summary of challenges for the current string interpolation based approach: The user here entered a log expression to be evaluated, and was first of all surprised by the the At the moment you can't with LogPoints as we don't provide any "magic" for the users, which We can sure find a way to do object serialization, but my take is that we'll eventually end up implementing the the console apis in the Node DA to get parity Based on this I still think we have to meet node developers they where are with When the Edge DevTools team implemented Log/TracePoints they ended up reaching the same conclusion by offloading to The value prop for LogPoints is that they are a quick and easy way to inject log messages into your application code, and we have to be at least in parity with the existing methods on the logging functionality side, and for the experience we have to significant better then the typical flow (in Node) by injecting a log, saving the file, and restarting the process |
Formatting objects nicely is definitely critical. I think that passing the objects as arguments to console.log is the only way to do that. Without breaking at the logpoints, the DA will never have a chance to evaluate the arguments itself. |
The DA can format the stuff that goes to the debug console however it wants to, but it can't change what goes to stdout in the terminal after it sets up the console.log conditional BP |
And I think it's a separate discussion from which syntax to use. Passing multiple arguments to console.log enforces a space between each one so it could add extra spaces in some cases, with the {} syntax. |
@weinand Please see comments. |
Instead of using the string value of interpolated expressions, we could easily use the object value instead. This may result in "nicely formatted objects", but please keep in mind the following shortcomings:
|
@weinand @roblourens I'm wondering if we could follow the best practice from MDN that recommends doing a https://developer.mozilla.org/en-US/docs/Web/API/Console/log#Logging_objects I think splitting expressions onto multiple lines would be a great starting point. |
If we end up doing our own object serialization, would we end-up duplicating |
JSON.parse is lossy for things that can't go into JSON like classes, or functions, or NaN. And it doesn't handle circular references, we would need to write our own code for that. It could be slow on large objects. |
So what could be a solution here? Breakdown expressions and forward to console.log as arguments?
Use formatting options for expressions:Chrome/Node supports print-if style formatting. Se https://developers.google.com/web/tools/chrome-devtools/console/console-reference#format-specifiers and https://nodejs.org/api/util.html#util_util_format_format_args
https://developers.google.com/web/tools/chrome-devtools/console/console-write |
Yes, using The current approach is to delegate to console.log(...) and I suggest that we continue to do this as long as possible. So On the other hand: since we control both sides of the pipe, we could easily use just Another issue we should address is the unhelpful source location shown in the debug console. Currently all log output shows "VM123" as the source link because all console.logs are evaluated in an unnamed script (which we name by using the script id). Instead we could feed the Log Point's source location into the Output event's source attribute. One way to do this would be to tunnel the location as an additional argument through the console.log call. |
@weinand @roblourens How do we best move forward? Open a new issue, so we can add it to the backlog? I didn't know the log messages would show up with a VMsource. We should see if can get that fixed in the next iteration. |
Please create new issues in VS Code for:
I've already tried Breaking the So for this logMessage:
you would get something like the following ('>' means expandable tree):
We need to explore more options (including improving the debug console). But for this we'll have to wait until for @isidorn to return from vacation. |
%o isn't supported yet, but easily can be: https://github.com/Microsoft/vscode-chrome-debug-core/blob/master/src/chrome/consoleHelper.ts#L112 |
Issues opened. Closing. |
Change LogPoints's logMessage to be evaluated as an expression and not a string, so LogPoints like
LogPoint('products', products)
can be set.