Add support for subscribing to topics by regex #433
Conversation
demetris-manikas
changed the title
tests/kafka/kafka.test.ts
Outdated
| @@ -20,6 +20,9 @@ const kafkaConfig: KafkaConfig = { | |||
| logLevel: logLevel.NOTHING, // FOR TESTING | |||
| } | |||
| const kafka = new KafkaJS(kafkaConfig) | |||
| const patternTopic = /dbos-test-.*/; | |||
There was a problem hiding this comment.
Maybe make this in as a RegExp object?
There was a problem hiding this comment.
It is. Try doting it. I can make it explicit if you wish.
There was a problem hiding this comment.
How is it that you prefer it ? new RegExp('dbos-test-.*') or new RegExp(/dbos-test-.*/)?
they all do the same. Javascript. More ways to do the same thing than anyone needs... :)
There was a problem hiding this comment.
Yeah, making it explicit with the latter notation is good
demetris-manikas
force-pushed
the
kafka-consume-support-pattern-topics
branch
from
0a81688
to
5d0b597
Compare
|
Rebased and waiting review |
tests/kafka/kafka.test.ts
Outdated
| DBOSTestClass.patternTopicResolve(); | ||
| } | ||
| } | ||
| return DBOSTestClass.patternTopicPromise; |
There was a problem hiding this comment.
Don't return promises from operations (it can cause failures because they're not serializable), just await them like in the other tests
There was a problem hiding this comment.
It is not a big deal here cause it is just a test but why await? the caller awaits if he cares to. I ve learned not to await unless needed (can very easily create synchronous code where you don't expect it ).
As you wish though.
There was a problem hiding this comment.
I also have learned to use explicit returns. (burned sometimes for not doing it in nestjs).
|
Suggestions applied |
|
Happy to help. |
|
I think that would also be a good change, if slightly more involved |
|
Shoot.. The 'topic' way is deprecated. Should have been an array from the start, I guess it is too late to change the Decorator so I have to create another one or have topic defined as string | RegExp | Array<string | RegExp> which is not that bad except for the variable name. Apart from that the implementation is quite easy. |
|
Defining |
kafkajs provides support for it so why not expose it?
This can be extended so as to receive an Array<string | RegExp> instead of just one.