Skip to content
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

Implement SignalWithStart as a synchronous Nexus operation #1791

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pdoerner
Copy link
Contributor

What was changed

Implemented a Nexus operation to wrap SignalWithStart
Refactored the SignalWorkflow Nexus operation to use the same options as SignalWithStart

Why?

Checklist

  1. Closes

  2. How was this tested:

  1. Any docs updates needed?

@pdoerner pdoerner requested a review from bergundy January 24, 2025 23:40
// Workflow function to map this operation to. The operation input maps directly to workflow input.
// The workflow name is resolved as it would when using this function in client.ExecuteOperation.
// GetOptions must be provided when setting this option. Mutually exclusive with Handler.
Workflow func(workflow.Context, I) (O, error)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We said we wouldn't enforce the operation and workflow to have the same output types since we're not binding the operation to the workflow's result, we're returning immediately after signaling.

func NewWorkflowSignalWithStartOperation[I any](
name string,
getSignalInput func(context.Context, I, nexus.StartOperationOptions) (WorkflowSignalInput, error),
workflow func(workflow.Context, I) (nexus.NoValue, error),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The workflow can return whatever it wants, it's not part of the operation output though.

// ensure idempotency. The operation is complete as soon as the signal is delivered and returns no value.
//
// NOTE: Experimental
func NewWorkflowSignalWithStartOperation[I any](
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be more convenient to have separate options for signal and signal-with-start and only require a single handler method.

I'd go for something like:

type WorkflowSignalWithStartInput struct {
	Workflow any
	Signal string
	WorkflowArgs []any
	SignalArg any
	StartWorkflowOptions client.StartWorkflowOptions
}

func NewWorkflowSignalWithStartOperation[I any](
	name string,
	handler func(context.Context, I, nexus.StartOperationOptions) (WorkflowSignalWithStartInput, error),
) nexus.Operation[I, nexus.NoValue]

The input of the operation is unlikely to map directly to the workflow input, and the workflow output doesn't get linked to the operation output.
There's no way to get a signal's type signature in the Go SDK, so there's not much value in exposing having this method be generic apart from just passing the operation input to the handler function.

return nil, err
}
ctx = context.WithValue(ctx, internal.NexusOperationLinksKey, links)
// NewWorkflowSignalWithStartOperation is a helper for creating a synchronous nexus.Operation to deliver a signal to a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having NewWorkflowSignalWithStartOperation be synchronous is not very intuitive for users, I would expect a signalWithStart operation to be an async operation that attaches to the existing workflow

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, signal-with-start is about the signal, not about the workflow run. Users will typically use this API to ensure that there's a workflow running to receive the signal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants