rotundasoftware / parcelify

Add css to your npm modules consumed with browserify.
MIT License
251 stars 20 forks source link

Make logging optional #10

Closed jsdf closed 9 years ago

jsdf commented 10 years ago

Currently parcelify prints out stuff to the standard streams via npmlog, with no option (that I'm aware of) to suppress it. That's fine when it's being run as a command, but for API use it would really be best if API consumers were able to attach their own logging instead.

Ideally that would mean that events would just be proxied to the parcelify instance, and logging only attached in cmd.js, but even just the option to supress logging would help (though kind of ugly, API wise).

I'm happy to put together a PR, just wanted to know which option would be preferred.

dgbeck commented 10 years ago

HI James! Thanks for the feedback. Curious what the use case is here?

Decoupling logging by listening for relevant events in cmd.js is a cool idea. One concern is that there are some cases, like unimportant informational log entries for debugging, where it is useful log something but we don't necessary want to add complexity to the parcelify event API in order to do so.

Probably better to go with second approach you mention we where pass the logging level through to npmlog. Seems reasonable enough to me but can you fill in use case for good measure?

Thanks!

jsdf commented 10 years ago

One example: when running build tasks in a worker process stdout and stderr can be used to pass progress and error information back to the supervisor process. Without control of what is actually being written to stderr, there is no way to format error messages consistently (short of parsing the different formats of logging which might be output). This would also be the case when logging output to a log file (where it is desirable to have consistently formatted output for later parsing).

More generally as an API consumer I should have control of what is being written to the standard streams by modules (as they are 'global', in a sense).

dgbeck commented 10 years ago

Good points. OK, if you can cook up a PR for a logLevel we'll push it through. Looks like npm has the same option. We can follow suit, more or less, with accepted values, although seems like we can simplify a bit. How about.

"silent", "error", "warn", "info", "verbose"

ghost commented 9 years ago

http://www.faqs.org/docs/artu/ch01s06.html#id2878450 is a good rule of thumb here. It's much easier to enable a feature than to disable one.

dgbeck commented 9 years ago

Hard to take issue with that logic!

We should be good here.. this change was made in #11 and default logLevel is 'warn' so only important things should be showing up.