kafka-streams icon indicating copy to clipboard operation
kafka-streams copied to clipboard

Published 20 hours ago • verified publisher icon nodefluent

Updated types from unknown and removing unused variables

Open rob3000 opened this issue 5 years ago • 7 comments

  • Starting to move types across.
  • Tests will fail due to types not being correct in node-sinek

Which leads me to the next part of updating node-sinek in kafka-streams. In node-sinek i recently removed deprecated client versions and removed node-rdkafka but made KafkaJS as the primary connection to kafka, so some of the items in Kafka-streams will need to change. I thought i'd get these type changes up first and then look to remove/replace the removed clients e.g https://github.com/nodefluent/kafka-streams/blob/a9a329ae07e67565d5467dac4275b88a48a1ad15/src/lib/client/JSKafkaClient.ts#L121,L123

rob3000 avatar Aug 24 '20 02:08 rob3000

Awesome job!

Which leads me to the next part of updating node-sinek in kafka-streams. In node-sinek i recently removed deprecated client versions and removed node-rdkafka but made KafkaJS as the primary connection to kafka, so some of the items in Kafka-streams will need to change. I thought i'd get these type changes up first and then look to remove/replace the removed clients e.g

Makes sense!

wtrocki avatar Aug 24 '20 07:08 wtrocki

@wtrocki whats the best way to introduce those sinek changes? Assuming merge this in ASAP then update?

rob3000 avatar Aug 24 '20 07:08 rob3000

IMHO rule of thumb is to never break/merge broken changes to master. Especially when we are working on some major rehaul we want to ensure that unit and integration tests work. We need dev release of node-sinek and use it here with some typing fixes. Since you have been working with Node-sinek it makes sense to fix all problems there and do dev release that will be used here.

Also see that node-sinek tests are failing: https://github.com/nodefluent/node-sinek/runs/1019670873

wtrocki avatar Aug 24 '20 08:08 wtrocki

@rob3000 Sorry for getting back to you so late. I'm back to the kafka game. Do you have anything blocking here ?

wtrocki avatar Sep 22 '20 13:09 wtrocki

@wtrocki the only item outstanding is the failing tests so want to make sure the conversation to Typescript hasn't broken anything

rob3000 avatar Sep 22 '20 14:09 rob3000

Currently need the following PR: https://github.com/nodefluent/node-sinek/pull/161 to be merged to include an alpha build of sinek for this PR.

rob3000 avatar Sep 24 '20 06:09 rob3000

Finally managed to spend some time looking into the failing tests 🎉 So with the alpha version of sinek, it now uses kafkaJS as the client. This means some of the tests needed to change: https://github.com/nodefluent/kafka-streams/pull/167/commits/c350a33cc4866c21a250c77535179bc49270cda7#diff-2b3239e3f6bb8003ba55ac789c8a8812R100-R110

Let me know thoughts?

rob3000 avatar Oct 02 '20 06:10 rob3000