Move CountertopWorker to TypeScript #188
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
cc @louh this PR actually represents the migration of a js class to typescript (check out the final commit for the meat and potatoes) |
Instead of defining log levels we actually just want to define the shape of the expected logger class. I want that shape to adhere to the defaults provided by winston [1] and pino [2], which both support "debug, info, warn, error" (they support others, but those are the overlap. Issue #153 [1] https://github.com/winstonjs/winston#using-logging-levels [2] https://getpino.io/#/docs/api?id=logger
slifty
force-pushed
the
153-typescript-countertop-worker
branch
from
862ec6b
to
c93f0f2
Compare
Codecov Report
@@ Coverage Diff @@
## main #188 +/- ##
==========================================
- Coverage 88.96% 82.90% -6.06%
==========================================
Files 22 27 +5
Lines 435 509 +74
Branches 73 93 +20
==========================================
+ Hits 387 422 +35
- Misses 46 83 +37
- Partials 2 4 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
slifty
force-pushed
the
153-typescript-countertop-worker
branch
from
c93f0f2
to
e98b831
Compare
The test regex for javascript was a little extra -- it would run tests on any file whose *path* contained test or spec after __test__. This could result in situations where, say, a fixture directory had the same name as a test file and also contained javascript, causing those files to be considered tests. This fixes that and also adds some parallel regex for typescript based tests
slifty
force-pushed
the
153-typescript-countertop-worker
branch
12 times, most recently
from
e9236a0
to
355e0a5
Compare
We don't want to know the test coverage of test code (e.g. fixtures).
slifty
force-pushed
the
153-typescript-countertop-worker
branch
4 times, most recently
from
7957344
to
0ec7447
Compare
|
So here's my thinking:
Which is to say... I don't think I'm going to use this PR as an opportunity to address the poor test coverage even though codecov is upset about it. Once the TS conversion is complete I think the project will be in a good place to take a hard look at the kafka dependency, at which point it will be much easier to write more comprehensive tests for the worker. |
The concept of an abstract appliance / an appliance interface had existed in the TVKitchen base class package [1]. We had already wanted to bring it into the countertop repository, but with TypeScript we also get to clean up a lot of "faked" abstract behavior which should make it much easier for developers to build their own appliances. Issue #153 Move to Typescript Issue #154 Base should be baked into countertop
Jest was still using ts-jest to process typescript tests, which can cause problems in cases where the tests interacted with javascript modules. This removes that in favor of using our babel config. Once we're fully typescript we can remove babel and re-add ts-jest.
This is the first real class to be converted to Typescript. It required some new type definitions for kafka, and unfortunately also had to include some temporary lint-disabling since the worker relies on stream and stream doesn't yet have types. Issue #153 Move to Typescript
slifty
force-pushed
the
153-typescript-countertop-worker
branch
from
0ec7447
to
5f155c6
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR moves the CountertopWorker class to TypeScript.
It also brings the AbstractAppliance from our @tvkitchen/base over to this project (and converts it to TypeScript as well)
Related to #153
Related to #154