-
Notifications
You must be signed in to change notification settings - Fork 108
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
Add prefix output #975
base: main
Are you sure you want to change the base?
Add prefix output #975
Conversation
sirenkovladd
commented
Nov 20, 2023
@rictic can you review this? |
@@ -50,6 +52,15 @@ export class DefaultLogger implements Logger { | |||
this.#rootPackageDir = rootPackage; | |||
this.#diagnosticPrinter = new DiagnosticPrinter(this.#rootPackageDir); | |||
this.console = ourConsole; | |||
this.outputWrite = process.env['WIREIT_LOGGER_PREFIX'] ? this.outputWriteDebug : this.outputWriteDefault; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's put this logic on our Console
class as a new method that takes an output event and writes it out to the proper underlying stream, using the presence / absence of the env variable to determine whether to use the prefix. That way the implementation lives in a single place.
} | ||
|
||
private outputWriteDebug(this: void, stream: NodeJS.WritableStream, event: Stdout | Stderr, label: string) { | ||
stream.write(event.data.toString().split('\n').map(line => line.trim() ? `[${label}] ${line}` : line).join('\n')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't quite right, because output events don't have to map cleanly onto lines. For example, you could have a program that logs "abc" with a pause between each letter. What we want to log here is:
[scriptname] abc
But as you've written this, I believe wireit would log:
[scriptname] a
[scriptname] b
[scriptname] c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem to work in the current version
https://stackblitz.com/edit/stackblitz-starters-eweown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that looks like a bug as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes indeed, but even if it worked (I can send a PR after the discussion), what should be output in the example I posted
I see two possible options, or output what is sent
daefbq
decfq
q
deafq
dbefcq
or save the line until I get a transition to a new line (which is, of course, a bad option, since the user needs an answer immediately, and not waiting for the end of the line)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean. Looking into it our options a bit:
- We could implement a full TUI and render a separate window for each server, each with its own scrollback. I'd like to explore this for Wireit anyways longer term, as this would let us do some other interesting things at the UI level. However, that'd involve bringing in at least one sizeable new dependency, and likely a lot of additional code. Not in scope for a small change, and potentially not even something we're willing to maintain as part of Wireit.
- We could use ANSI escape codes to update previous lines. In theory, this would be ideal, letting us have multiple servers all writing at the same time without stepping on each others' lines, and the complexity and overhead would be manageable (just need to track newlines written, and characters written per line), but unfortunately ANSI control is based on window position, not terminal output position. So you can only modify lines that are up to N lines back in the scrollback, where N is the height of the user's terminal window.
- We could remember whether the previously write was by a server and didn't end in a newline, and if so, only create a newline if the next write is by something other than that server. This seems like a good trade off of correctness vs complexity.
With strategy 3, the output of your example would be:
[process1] a
[process2] de
[process1] b
[process2] fq
[process2] d
[process1] c
[process2] ef
[process1] q
<... and so on>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super necessary feature for complex service dependency chains. And it looks like this PR has stalled due to the complexity of correctly handling this relatively rare scenario. Couldn't we just add a caveat in the documentation for the env variable letting users know about the effect that would have on their log output? For dev tools, the correctness requirements are often less stringent. For example, the wildly popular npm-run-all
package has a printLabels
option that actually stops colorizing output. See https://github.com/mysticatea/npm-run-all/blob/master/docs/npm-run-all.md#known-limitations.
I think this feature is too important to delay for this reason.
Co-authored-by: Peter Burns <[email protected]>