Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
The other language SDKs don't use a specific converter for RawValue - rather, it is expected that RawValue always has it's inner value directly passed through as the payload (ex: what if the user wants to explicitly set some other unusual metadata encoding while using raw value? This current solution prevents that).
So, for example, in the Python SDK, the composite converter explicitly handles things typed as RawValue before delegating to other converters: https://github.com/temporalio/sdk-python/blob/b69ac9258bc13c4a3d454f4c4a8d98b4f834babe/temporalio/converter.py#L377
There was a problem hiding this comment.
Thanks for mentioning it.
I thought about this way as well, but there're a few things that bother me:
- hardcoded check
- not possible to use original DataConverter without it
- not possible to use RawValueConverter in your own DataConverter as another converter
- it works only with type-hints:
-- client side:$stub->getResult(RawValue::class)
-- workflow incoming argument:function my_workflow(RawValue $v)
-- workflow outgoing return type:function my_workflow(RawValue $v): RawValue
Wouldn't it better to keep DX as max as possible handing this things tricky?
ex: what if the user wants to explicitly set some other unusual metadata encoding while using raw value? This current solution prevents that
We have DataConverter which may apply any actions on a value, setting custom user encoding as well, isn't it?
Anyway, I have rewrote solution, could you please check again? We may discuss my message later if you think it has value.
There was a problem hiding this comment.
Only working with type hints is kind of the point - this is really just purely a type-level declaration that something should be passed through as-is. After all, the [de]serialization always requires us to say what we're serde-ing, so I don't see that as an issue.
not possible to use original DataConverter without it
Not sure I follow this one, as the original/default DC in all SDKs should be a composite with the various things we support out of the box, so this composite also supports the RawValues
not possible to use RawValueConverter in your own DataConverter as another converter
Yeah, that's a downside - but implementing it is pretty obvious / trivial. We could always expose a static helper for that purpose if desired.
What was changed
Why?
Closes #574
Checklist
Closes
How was this tested: