-
Notifications
You must be signed in to change notification settings - Fork 127
Add RawValue support for non-converted Payloads #1664
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
I believe we need to return |
I'm really not clear how For now, I'd focus on the outbound path only, simply to support return value of built-in queries. For inbound payloads, that might have to wait only we add support for type hints on receivers. |
62de924
to
cec9e55
Compare
cec9e55
to
573c2ca
Compare
I've written this in the PR description:
Let me know if this makes sense to you. |
@@ -4,6 +4,7 @@ | |||
*/ | |||
import * as wf from '@temporalio/workflow'; | |||
import type { EnhancedStackTrace } from '@temporalio/workflow/lib/interfaces'; | |||
import { defaultPayloadConverter } from '@temporalio/common/lib/converter/payload-converter'; |
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.
Unused import
@@ -22,6 +22,7 @@ import { | |||
WorkflowReturnType, | |||
WorkflowUpdateValidatorType, | |||
SearchAttributeUpdatePair, | |||
RawValue, |
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.
Unused import
@@ -315,14 +317,14 @@ export class Activator implements ActivationHandler { | |||
name, | |||
description: value.description, | |||
})); | |||
return { | |||
return new RawValue({ |
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.
It's sad we're loosing type safety check here… There's nothing ensuring that the arg to RawValue complies with the expected type definition.
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.
See my suggestions elsewhere that adds back type safety.
* A payload that belongs to a RawValue is special in that it bypasses user-defined payload converters, | ||
* instead using the default payload converter. The payload still undergoes codec conversion. | ||
*/ | ||
export class RawValue { |
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.
For the sake of type safety, I think we should add an optional generic parameter on RawValue, and save that generic parameter using the type brand pattern. Also, we should accept passing in an optional payload converter, i.e. in cases where user want to proce a specific payload converter (not the default one, and not the one they configured on the worker or client).
export declare const rawPayloadTypeBrand: unique symbol;
class RawValue<T = unknown> {
private readonly _payload: Payload;
//
private readonly [rawPayloadTypeBrand]: T = undefined as T;
constructor(value: T, payloadConverter: PayloadConverter = defaultPayloadConverter) {
this._payload = payloadConverter.toPayload(value);
}
get payload(): Payload {
return this._payload;
}
}
@@ -386,7 +388,7 @@ test('Query workflow metadata returns handler descriptions', async (t) => { | |||
|
|||
await worker.runUntil(async () => { | |||
const handle = await startWorkflow(queryWorkflowMetadata); | |||
const meta = await handle.query(workflow.workflowMetadataQuery); | |||
const meta = (await handle.query(workflow.workflowMetadataQuery)) as temporal.api.sdk.v1.IWorkflowMetadata; |
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.
You should not need a cast here.
return this.getStackTraces() | ||
.map((s) => s.formatted) | ||
.join('\n\n'); | ||
return new RawValue( |
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.
return new RawValue( | |
return new RawValue<string>( |
}, | ||
description: 'Returns a sensible stack trace.', | ||
}, | ||
], | ||
[ | ||
'__enhanced_stack_trace', | ||
{ | ||
handler: (): EnhancedStackTrace => { | ||
handler: (): RawValue => { |
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.
handler: (): RawValue => { | |
handler: (): RawValue<EnhancedStackTrace> => { |
}, | ||
description: 'Returns a stack trace annotated with source information.', | ||
}, | ||
], | ||
[ | ||
'__temporal_workflow_metadata', | ||
{ | ||
handler: (): temporal.api.sdk.v1.IWorkflowMetadata => { | ||
handler: (): RawValue => { |
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.
handler: (): RawValue => { | |
handler: (): RawValue<temporal.api.sdk.v1.IWorkflowMetadata> => { |
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.
We're almost there, but still a bit more.
What was changed
Added
RawValue
support for non-converted Payloads - a thin wrapper aroundPayload
.RawValue
is intended to be used for payloads that bypasses users custom payload converters in favour of the default payload converter.TS does not include type hints or similar when converting from a payload back to its original representation. As such, the best we can do is just send the payload as is and have users convert it back to its original representation.
The existing builtin queries now return a
RawValue
that wraps their former returned values. The return types of the builtin queries however, maintain their existing type (this is because receivers of these queries will get the payload itself, not theRawValue
). This is breaking change as consumers of these builtin queries will need to make sure they decode the builtin payloads correctly.Closes [Feature Request] Support "RawValue" non-converted values #1629, [Feature Request] Built-in query responses should use "RawValue" #1630
How was this tested:
Added short integration test, modified the existing built-in metadata test.
Any docs updates needed?
Possibly. Maybe for the change to the built-in metadata query.