KafkaJS v2 #4427
KafkaJS v2 #4427
Conversation
| @@ -23,3 +23,5 @@ | |||
|
|
|||
| /definitions/npm/sequelize_*/**/* @jedwards1211 | |||
|
|
|||
| /definitions/npm/kafkajs_*/**/* @edoardo-bluframe | |||
| /definitions/npm/react-helmet_v6*/**/* @edoardo-bluframe | |||
There was a problem hiding this comment.
Check out this to add code owners in the respective definition instead for the service to do it's thing
| }); | ||
|
|
||
| it("uses log levels", () => { | ||
| const logLevels = [ |
There was a problem hiding this comment.
This might better test the expectation, your current code block doesn't actually test anything at all
| const logLevels = [ | |
| const logLevels: number[] = [ |
| it("creates Consumer instance", () => { | ||
| const kafka = new Kafka({ | ||
| clientId: "my-app", | ||
| brokers: ["kafka1:9092", "kafka2:9092"], |
There was a problem hiding this comment.
Suggestions for the constructor, you might want to exercise the options,
- can it accept all the expected options
- what if you pass in the invalid values for the option
- what if you pass in an option that doesn't exist?
Then once constructed with kafka returned to you, does it has the functions and properties you expect but at the same time, does it do things you don't expect?
There was a problem hiding this comment.
Definition itself looks good, I can't fault it's structure.
Regarding tests though, I see that you've approached this with use cases in mind which is good, shows that it works as you expect. But the core thing about tests is that you're trying to exercise the definition to prove 2 things, what you've written is actually valid and if someone were to change your definition it would break expectedly.
You'll need some // $FlowExpectedError samples in some places, the fact that you don't have any begs the question, how do we know that your types are actually strict and that they're not just any and you can actually call any function against it? Here is an example of what that might look like, though you don't need to go to the extreme levels of testing everything as I understand it can be tedious, but enough testing so we don't assume a type can be used in the wrong way.
If a nutshell, if you have a block that proves it does this you should also have another block similar that states it shouldn't be able to do this
Links to documentation: https://www.npmjs.com/package/kafkajs
Link to GitHub or NPM: https://github.com/tulios/kafkajs
Type of contribution: addition