-
Notifications
You must be signed in to change notification settings - Fork 2
feat: Refactor payload encoding/decoding for extensibility #17
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
base: main
Are you sure you want to change the base?
Conversation
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'm all for this change. Do you mind adding a little bit of context for the bug or use case this PR addresses? It'll help other people if they had this issue also find the fix and what version they'll be needing.
static decodePayload<T>(s: string | null) { | ||
return JSON.parse(s ?? "{}")._ as T; | ||
static decodePayload<T>(s: string | null | unknown) { | ||
return JSON.parse((s as string) ?? "{}")._ as T; |
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.
This will probably need a try catch around it now, since non-strings really mess up the JSON.parse (it probably always needed one, but the addition of an unknown
makes it way more likely to trigger this footgun)
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.
Can you catch & rethrow a useful error here? This way the default encoders set a good example
Thank you for your feedback, I just want to offer a way to conveniently data serialization, which can be useful for MongoDB users (the ability to find a job by its payload) new Queue(driver, name, {
decodePayload: payload => payload,
encodePayload: payload => payload,
}) |
16074b0
to
d5f9e16
Compare
I think the approach you have here w/ instance based encode/decode is the correct way to do this. The tests are failing, but that is likely a "me" problem, because we haven't had to update the tooling and cut a release in a while. |
Demo (before/after)