Change type of value attribute of KafkaMessage

[fredefox]

References #642

I ran into the issue that the type declaration file does not match runtime behaviour. I suspect this is due to the type declaration being wrong as opposed to a runtime bug. C.f. the linked issue.

[fredefox]

I should add that this is obviously to be considered a breaking change as any project that uses TypeScript and relies on this information may now start seeing type errors.

[Nevon]

I should add that this is obviously to be considered a breaking change as any project that uses TypeScript and relies on this information may now start seeing type errors.

Hmm, this is a tricky one. Historically we've allowed technically breaking changes to type definitions when the types didn't match the runtime behavior anyway. Essentially the argument was that it's better to treat it as a bug fix and get it rolled out to as many people as possible, than make a major release that will take longer to adopt, since anyway people will have unexpected behavior without the fix. However, in practice those changes have usually been for things that affect relatively few people, but the message value is something that literally everyone will be using, so every single TS user will now have a breaking change on their hands. I'm not sure what the right thing to do in this case is.

[ankon]

If I understand correctly though then the current behavior is that the message could be null (1 and others), but code written against the current type definition won't be guarding against this. So, while this "breaks" code, it actually breaks it in the right direction by telling you that you need to do something here to be "safe". Pretending that the message cannot be null in the types doesn't sound correct to me. It would be good to point this out very loudly in the release notes though, and maybe check what the impact here is.

FWIW: I tried this change with our codebase, and as expected we do see the error from the compilation popping up. But, we're using a wrapping library (which validates schemas of messages etc), and the fix for us was to add that needed null-check and report an error/skip the message as we don't expect tombstones to come by.

[fredefox]

If I understand correctly though then the current behavior is that the message could be null

That's correct!

I'm not sure what the right thing to do in this case is.

Personally I'd be fine with either. I just wanted you to be aware of the issue. I suppose the answer to the question depends on what you define to be a "breaking change". Since it may cause build pipelines to fail that did not previously fail, then I would consider that a breaking change. Of course since it's only a refinement on the types it does not have any impact on runtime behaviour. So no breakage there.