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

grab chalk and use it to make the build and engine loading process pr… #565

Merged
merged 1 commit into from
Nov 25, 2016

Conversation

geoffp
Copy link
Contributor

@geoffp geoffp commented Nov 24, 2016

I wanted it to be pretty, so I did stuff to it.
screenshot from 2016-11-23 23-12-03

@raphaelokon
Copy link
Contributor

raphaelokon commented Nov 24, 2016

I switched CLI from colors to chalk the other day as well. In future would be good to log verbosely via a CLI flag … I use a small event-driven logger for that in the CLI …

https://github.com/pattern-lab/patternlab-node-cli/blob/master/bin/utils.js#L10-L25

@tburny
Copy link

tburny commented Nov 24, 2016

I'd like to suggest replacing all console.log statements with more robust logging - with no exceptions.
This way logs become parsable by standard command line tools like cut, sed/awk, etc. We get some timings for free, to see which step takes how long.

In summary I like the idea :)

@geoffp
Copy link
Contributor Author

geoffp commented Nov 24, 2016

@raphaelokon, that looks nice, but what's the benefit of using an event bus for logging, as opposed to just calling a simple logging function directly?

@tburny, are you after some kind of delimiter that a parser can grab onto, or timestamps for every log entry, or what? Gulp does something that, and I like that it tells you how long tasks take.

@raphaelokon
Copy link
Contributor

@geoffp The main benefit is that the event listener can decide what to do with the logs that were emitted. I do not have to log them in place, but can decide how to pipe them into different listeners (eg. writing log files, console.log in specific format etc).

@tburny open to formats here.

@raphaelokon
Copy link
Contributor

@geoffp console.log is also blocking on tty terminals, like OS X due to limited buffer size (I think 1KB or so)

@bmuenzenmeyer
Copy link
Member

I've endeavored to keep dependencies low - hence the current pretty ghetto implementation. I'd like feedback from users regarding additional, perhaps cosmetic dependencies.

I also like the idea @raphaelokon proposed about a better logger. Seems like a more robust way to emit and listen to events of differening importance to the user / use case.

@bmuenzenmeyer bmuenzenmeyer mentioned this pull request Nov 25, 2016
4 tasks
@bmuenzenmeyer
Copy link
Member

I've come around to this - I created #566 to finish it up.
Another issue / pr could be to replace console.log with something better across the board.


console.log(
chalk.bold('\n====[ Pattern Lab / Node'),
`- v${packageInfo.version}`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like what this adds to output Geoff

@bmuenzenmeyer bmuenzenmeyer merged commit a18c3bb into dev Nov 25, 2016
@bmuenzenmeyer bmuenzenmeyer deleted the pretty-engine-loading branch November 26, 2016 11:09
@bmuenzenmeyer
Copy link
Member

@geoffp console.log is also blocking on tty terminals, like OS X due to limited buffer size (I think 1KB or so)

@raphaelokon would you recommend utilizing your event logger in this case then?

@raphaelokon
Copy link
Contributor

The event-based logger just delegates the logging part to the handler function(s) you call … if you call console.log in a handler, it would also block afaik.

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.

4 participants