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

Add prefix output #975

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

sirenkovladd
Copy link

{
"wireit": {
  "test": {
      "service": true,
      "command": "echo 'line1\nline2\nline3\nline4'"
    }
  }
}
❯ WIREIT_LOGGER_PREFIX=1 npm run test

> monitoring@1.0.0 test
> wireit

[test] line1                               
[test] line2
[test] line3
[test] line4
❌ [test] Service exited unexpectedly
❌ 1 script failed.

@sirenkovladd
Copy link
Author

@rictic can you review this?

README.md Outdated Show resolved Hide resolved
@@ -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;
Copy link
Member

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'));
Copy link
Member

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

Copy link
Author

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

Copy link
Member

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

Copy link
Author

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)

Copy link
Member

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:

  1. 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.
  2. 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.
  3. 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>

Co-authored-by: Peter Burns <rictic@gmail.com>
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.

None yet

2 participants