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

Allows log without disturbing progress bars #57

Closed
wants to merge 1 commit into from

Conversation

dlvoy
Copy link

@dlvoy dlvoy commented Jan 6, 2020

Using console.log together with cli-progress while progress bars are running disturbs bars and make log contents disappear under rendered bars.

This enhancement adds log method on MultiBar which blends nicely with standard rendering pipeline of progress bars.

@AndiDittrich
Copy link
Member

Dear @dlvoy

thanks for your contribution! currently i cannot merge your pull request because:

  • the lint checks failed
  • it only works for multi-progress bars in async mode (single bar, sync missing)
  • not suitable for production use -> such method should called debug or similar because there are a lot of edge cases where it won't work as expected

generally i would recommend to redirect the stderr stream to a second terminal to display the progress bars during development. the current terminal can then handle any kind of stdout messages with standard log utils.

best regards, Andi

@dlvoy
Copy link
Author

dlvoy commented Jan 6, 2020

TL;DR: Having something like beforeUpdateRendering(terminal) & afterStop(terminal) event handlers, configurable in options, would allow to handle it app-side. Would you accept that kind of PR instead of current log - or should I not bother more and just left it to the fork?


I understand your points but can argue on details :)

  • with lint - ...args vs arguments (https://eslint.org/docs/rules/prefer-rest-params), IMHO should be a warning - not error, anyway easy to fix
  • in the async mode it is needed the most: when you have multiple independent async tasks, some updating progress, other progress getting closed, and tasks already writing to console after finishing progress part
  • it is log because of console.log, for me debug would suggest that it is some kind internal debug info about progress bar itself

I know I can reconfigure everything for multi-console output etc. But I will argue the most users simply do not care or want to bother with multiple streams.
Default console object (https://nodejs.org/api/console.html) is configured on Node.js to output to both streams and its output gets broken by default when using it together with progress - in its current state.

I have spent half a day to work around this, and any other solution was worst:

  • I do not want to require end-user to redirect consoles only to see nice progress in between standard console.log output
  • Any other external solution without messing with lib was nastier, like keeping separate state for all progress bars outside and having custom log that stops progress, output logs and recreates them - even more edge cases - since in async we cannot simply output to stderr without messing with magic lib does with cursor, -dy etc. :D
  • I cannot simply plug that logic with current implementation externally

@AndiDittrich
Copy link
Member

please take a look into 6514f9e - it adds generic event support via EventEmitter - it should fulfill your requirements to add the logging as external component.

currently i've no idea how to add log/debug output in a reliable way (single bar, multi bar, sync, async,...). your provided solution has a lot of limitation and will only work in some use cases for multi-bars :(

@dlvoy
Copy link
Author

dlvoy commented Jan 7, 2020

@AndiDittrich wow! I am impressed! Basically replaced yesterdays handlers from #58 with Events redraw-pre & stop - and work like a charm! Thanks!

I also have no idea how to make it for all kind of bars and all use cases - just proposed what will solve subset of needs, which are there (YAGNI). But with Events i can simply do that user-space side - so i will close this PR.

Thanks again!

@dlvoy dlvoy closed this Jan 7, 2020
@AndiDittrich
Copy link
Member

v3.5.0 is out including the changes

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

Successfully merging this pull request may close these issues.

2 participants