💥 Adding links to Nexus signals#2114
Conversation
VegetarianOrc
left a comment
VegetarianOrc
left a comment
There was a problem hiding this comment.
Unsure if the CI failures are directly related to these changes, but please take a look and ensure they're passing.
| /** | ||
| * Backlink copied by the client from the SignalWithStartWorkflowExecutionResponse. | ||
| * Only populated by servers that support CHASM signal backlinks (1.31 and up); left unset | ||
| * otherwise. | ||
| */ | ||
| signalBackLink?: temporal.api.common.v1.ILink; |
There was a problem hiding this comment.
Is there a scenario where backLink and signalBackLink are both populated? If not we should remove this and use the one field.
| * {@link Client.workflow} to interact with Workflows. | ||
| */ | ||
| export class WorkflowClient extends BaseClient { | ||
| export class WorkflowClient extends BaseClient implements InternalWorkflowClientWithNexusLinks { |
There was a problem hiding this comment.
We shouldn't implement this interface here as it leaks the method onto client.workflow. I don't have a great suggestion for exposing signal to do the internal options dance though. One option might be to hide the method behind an internal symbol. Let's sync with @chris-olszewski and come up with what feels like the best path here.
There was a problem hiding this comment.
Changed as per our conversation!
| /** | ||
| * SDK-internal options used by the Temporal Nexus helpers to forward inbound Nexus links onto | ||
| * the signal request and to capture the backlink returned on the response. | ||
| * | ||
| * @internal | ||
| * @hidden | ||
| */ | ||
| readonly [InternalWorkflowSignalOptionsSymbol]?: InternalWorkflowSignalOptions[typeof InternalWorkflowSignalOptionsSymbol]; |
There was a problem hiding this comment.
We shouldn't leak these internal details here. instead we should define an internal interface in internal.ts that extends WorkflowSignalInput and adds this symbol.
| protected async _signalWorkflowHandler(input: WorkflowSignalInput): Promise<void> { | ||
| const dataConverter = this.dataConverter; | ||
| const context = this.workflowSerializationContext(input.workflowExecution.workflowId!); | ||
| const internalOptions = input[InternalWorkflowSignalOptionsSymbol]; |
There was a problem hiding this comment.
| const internalOptions = input[InternalWorkflowSignalOptionsSymbol]; | |
| const internalOptions = (input as InternalWorkflowSignalInput)[InternalWorkflowSignalOptionsSymbol]; |
Going along with my above comment about not leaking this symbol, this should be cast to an internal type here and the internal options retrieved from that. In my suggested change I made up a name, but feel free to use something more appropriate.
There was a problem hiding this comment.
Hopefully fixed as per our conversation!
| * NexusOperation history event back to the callee Workflow's event. No-op when the server did not | ||
| * return a backlink (older servers, or CHASM signal backlinks disabled). | ||
| */ | ||
| function pushBacklink(ctx: nexus.StartOperationContext, backLink: temporal.api.common.v1.ILink | undefined): void { |
There was a problem hiding this comment.
Nit: the backLink arg should probably not accept undefined. Call sites look like they have a side effect, but in reality may not do anything.
|
|
||
| export const pingSignal = workflow.defineSignal<[string]>('ping'); | ||
|
|
||
| export async function caller(endpoint: string, input: string): Promise<string> { |
There was a problem hiding this comment.
Let's remove the concept of the mode switch here and have separate caller workflows and operation handlers as needed for each test. Ideally we can colocate these right above where they're used to make it easier to see the full picture of what runs as part of the test.
|
|
||
| export async function caller(endpoint: string, input: string): Promise<string> { | ||
| const client = workflow.createNexusServiceClient({ endpoint, service: signalingService }); | ||
| const [mode, rest] = splitFirst(input, ':'); |
There was a problem hiding this comment.
My above comment stands about separating this out, but just want to call out that we should avoid encoding params as strings and instead use objects.
| async function fetchHistory(client: any, workflowId: string): Promise<temporal.api.history.v1.IHistory> { | ||
| return await client.workflow.getHandle(workflowId).fetchHistory(); | ||
| } |
There was a problem hiding this comment.
Let's remove this helper and just get a handle where necessary to get the history. There's a few places where we already get or have a handle that then call this helper.
Co-authored-by: Alex Mazzeo <alex.mazzeo@temporal.io>
Co-authored-by: Alex Mazzeo <alex.mazzeo@temporal.io>
…w-client behavior and allow passing a run ID.
VegetarianOrc
changed the title
2 participants
This adds links to Nexus signals - the receiving workflow has a link to the workflow sending the signal. and the sending workflow has a link to the receiving workflow.