feat(target-slack): adds slack webhook target and tests

ksafranski commented 7 years ago

Changes made with e1b8aac. LMK if there's anything else.

TomFrost commented 7 years ago

Beautiful!

MarkHerhold commented 7 years ago

Can I ask why we would use a heavily-bloated module (actually two, request-promise and request) to implement this? I really don't want these dependencies in my project when all I need is a slim logger. Why not just make this a third-party module?

I really don't think the core Bristol library is the right place for this.

Edit: I see that @Fluidbyte is part of the TechnologyAdvice org and hope that this wasn't quickly pushed because of this.

TomFrost commented 7 years ago

Hey @MarkHerhold -- Bristol pushes messages to Loggly too. The only difference in that case is that the loggly lib is left to the user to install manually. In this case, it's a little more cumbersome to say "To use the Slack target, install request" than "To use the Loggly target, install loggly".

I agree that Bristol should be lightweight-- I approved this because Bristol lazy-loads targets and formatters. While request would be downloaded for every install, it's never loaded unless you use this target.

With that said, though, I see your argument. Bristol should be just as attractive to space-conscious users as it is to resource-conscious users. The conundrum is in determining how fully featured Bristol should be out-of-the-box versus requiring users to find and install all the external libraries that would make it fully featured. Perhaps only allow formatters and targets that are both 1) intended primarily for the local machine (within reason), and 2) using only native libs (within reason), and make everything else external? I'd love your thoughts here.

(Regarding your Edit-- Kent and I do work together, but this wasn't a rush or a compromise to Bristol for the sake of our company. I've just always envisioned Bristol as eventually having targets for every endpoint you might hope for, and Slack seemed like a fantastic one -- especially for high-severity messages that you might want to inform a team about in real time)

MarkHerhold commented 7 years ago

@TomFrost In my opinion, I think everything should be a third party 'module'. To supplement onboarding and user experience, I would add a set of featured/recommended modules to the README of Bristol. Your users already have to install and write some code to write logs, so nothing is lost here.

e.g.

Bristol is an insanely configurable logging for Node.js

Getting Started with Bristol

Installing

npm i bristol bristol-slack --save

Use Bristol

var log = require('bristol');
log.addTarget('console'); // built-in console logger
log.addTarget(require('bristol-slack'), '<slack webhook>');

log.info("We're up and running!", {port: 3000});
log.error("Hello Slack!");

The Bristol Ecosystem

Slack - Send critical messages to Slack in realtime
Loggly - Use Loggly as a log target
Palin - A colorful console formatter
JSON Safe - Safely log circular references to JSON format

I think that the Slack target would be a good 'recommended' module and should be featured on the Bristol repo, but please don't force users to install the dependencies it if they don't need them for core functionality. I would say that even loggly shouldn't be a part of the core simply because it isn't essential even though it doesn't really bloat anything.

As another benefit of making Bristol module-focused is that you solve problems brought up by #35, where users want to add a dependency for their specific use-case but you don't want the project to suffer a performance hit overall. If you message the project as being extensible only by modules, it makes it clear that the 'right way' to add a feature is to write a module. This is the approach I tried to take with palin, because not everyone wants colors. 🌈 (even though those people are wrong)

As a whole, the Node.js ecosystem has elected to make everything small, focused modules. I think Bristol should follow suit. I'm not saying that all dependencies are bad, but they should be used sensibly and when a majority of users require it.

TomFrost commented 7 years ago

You make a shockingly good argument, @MarkHerhold ;-) The other benefit of that setup is that we could stop dancing around requiring certain modules (like the loggly lib) and just make it a direct dep of the add-on module. Bristol will always have bundled targets and formatters, but we'll go with this approach for anything large or dependency-laden.

@dannyrscott and @Fluidbyte, would either of you be interested in maintaining a bristol-loggly and bristol-slack module (respectively)? If not, I can create repos to host them. Per #31, using an external target or formatter will be as simple as passing in a string with the name of the module to be require()d. So no matter what configuration wrapper you may already have in place, using one of these would be as simple as npm install --save bristol-loggly and then using "bristol-loggly" as the target type.

TomFrost / Bristol