-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: handle kinesis parsing #69
Conversation
This is technically breaking, but since there have been only 2 consumers so far, all handled by our team, I thought we'd just do a minor bump. |
src/kinesis.ts
Outdated
JSON.parse( | ||
Buffer.from(record.kinesis.data, 'base64').toString('utf8'), | ||
) as Record<string, unknown>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think assuming base 64 is correct here, since that's a built-in Kinesis behavior. But, I don't think we should assume the message is JSON. That's not a Kinesis assumption.
"not assuming JSON" also aligns with the other abstractions in this package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, I think technically it's always a JSON object, but I get the point about aligning with the other abstractions already present. I don't think there's ever been the first occurrence where we don't just directly call JSON.parse
, right?
Nvm - we actually decide what to put on the stream.
🎉 This PR is included in version 5.5.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Motivations
The kinesis JSON object always comes in as base64 encoded data. All consumers need to parse the base64, and then call
JSON.parse
. We're taking care of that for consumers now, so they can start operating on a JSON object right away.