Add injectable logging - Githubissues

tvkitchen / countertop

The entry point for developers who want to set up a TV Kitchen.

https://tv.kitchen

GNU Lesser General Public License v3.0

6 stars 2 forks source link

Add injectable logging #94

Closed slifty closed 4 years ago

slifty commented 4 years ago

Description

This PR adds the ability to specify a logger when creating a new Countertop.

This PR removes reliance on winston.

This PR adds a default logger that uses console.log.

As part of the addition of inject-able logging, this changes the way appliance settings are passed to the countertop.

Due Diligence Checklist

[x] Tests
[x] Documentation
[x] Consolidated commits
[x] Relevant labels assigned
[x] Changelog(s) updated

Steps to Test

yarn test

Deploy Notes

yarn install

Related Issues

Resolves #91

chriszs commented 4 years ago

Presumably the logger in IAppliance goes away after this lands, right?

slifty commented 4 years ago

Regarding the IAppliance logger -- different repositories so they're independent (in theory someone could instantiate / invoke an appliance outside of the context of the countertop); maybe worth making the ConsoleLogger something added to base-classes which gets imported by both contexts?

That said... IAppliance probably shouldn't have an implemented logger anyway -- the idea of a default logger should be part of the appliance implementation / AbstractAppliance.

slifty commented 4 years ago

OK so I think this is good enough for now -- I hear you on the removing lines from the consoleLogger.js but there is enough reason to keep it that way for now that I will keep it there, and I expect it to become cleanup later.

The more long-term-important suggestions around things like API shape have been made so I think this is good to merge in!

chriszs commented 4 years ago

Yeah, I pretty much understood you were going to keep this design when I did my review. I think it's okay.