feat: Add Schedule API

[mjameswh]

What was changed

• Add Schedule API

Why?

• Add Schedule API

[bergundy]

Overall LGTM.
All of these new APIs need to be marked experimental.
I reviewed this very late at night so I may have missed things or asked trivial questions.

[bergundy]

Are we instantiating these interceptors per schedule instance? I wouldn’t do that, the interceptors shouldn’t be bound to a schedule.

[bergundy]

We should have probably avoided that for the client interceptors too.
Let’s think about this. I see that you did this for consistency.

[mjameswh]

I agree.

In that case, I don't think we need interceptor factories, do we?

If we can avoid these, then we can safely deprecate the previous approach in workflow client by defining:

export interface WorkflowClientInterceptors {
  calls?: (WorkflowClientCallsInterceptorFactory|WorkflowClientCallsInterceptor)[];
}

and then type checking either calls items are typeof object (new way) or function (deprecated way). We will have to continue supporting the previous approach for some minor versions.

[mjameswh]

Regarding Schedule interceptors, I will immediately remove factory and make

export interface ScheduleClientInterceptors {
  calls?: ScheduleClientCallsInterceptor[];
}

Makes sense?

[bergundy]

Probably get rid of the calls attribute and flatten it out:

export interface WorkflowClientInterceptor {
  // ... rename from WorkflowClientCallsInterceptor
}

// Maintain the export alias
export type WorkflowClientCallsInterceptor = WorkflowClientInterceptor;

export interface WorkflowClientInterceptors = {
  /** @deprecated */
  calls?: WorkflowClientCallsInterceptorFactory[];
}

interface ScheduleClientInterceptor {
}

export interface ClientInterceptors {
  workflow?: WorkflowClientInterceptors | WorkflowClientInterceptor[];
  schedule?: ScheduleClientInterceptor[];
}

[mjameswh]

Pretty much what I had in mind. I'll do the WorkflowClient refactor in a distinct PR though.

[dnr]

In the protos, ScheduleInfo is the stuff that can't be updated. It is not part of Schedule, which is everything that can be updated (everything else). ScheduleState is part of Schedule and can be updated. It's essentially the things that are expected to change and might change without an explicit update, as opposed to SchedulePolicies, which are not expected to change (but can be changed by the user).

I think at least using the same terms will help everyone avoid getting confused

[dnr]

I understand that we might want to tweak some names at this level, but it does seem confusing to have a ScheduleAction that corresponds to a ScheduleActionResult in the proto, while ScheduleAction in the proto is something else

[mjameswh]

Definitely agreed. That name is also confusing in regard to other types in the TypeScript's schedule API.

I'm considering this:

export interface ScheduleExecutionResult {
  /** Time that the Action was scheduled for, including jitter */
  scheduledAt: Date;

  /** Time that the Action was actually taken */
  takenAt: Date;

  /** The Action that was taken */
  action: ScheduleExecutionActionResult;
}

export type ScheduleExecutionActionResult = ScheduleExecutionStartWorkflowActionResult;

export interface ScheduleExecutionStartWorkflowActionResult {
  type: 'startWorkflow';
  workflow: WorkflowExecutionWithFirstExecutionRunId;
}

[lorensr]

This API hides all protos, right? So I don't see the issue with name conflicts. And generally prefer shorter terms, like:

export interface ScheduleAction {
  /** Time that the Action was scheduled for, including jitter */
  scheduledAt: Date;

  /** Time that the Action was actually taken */
  takenAt: Date;

  /** The result of the Action that was taken */
  result: ScheduleActionResult;
}

export type ScheduleActionResult = StartWorkflowActionResult;

export interface StartWorkflowActionResult {
  type: 'startWorkflow';
  workflow: WorkflowExecutionWithFirstExecutionRunId;
}

[mjameswh]

@lorensr I obviously prefer shorter names.

The problem here is that Describe, List, Create/Update, CompiledCreate/CompiledUpdate, recentActions, and runningActions all have distinct variants of the ScheduleAction* types. At this moment, we could deal with this by Omit<>ing or Pick<>ing some fields, but that would not work once we add other types of actions.

[bergundy]

This looks great, I don't have a lot of comments at this point and I would merge soon.
I'd follow up with a cleanup PR that adds an internal base class to all of the different SDK clients.

[bergundy]

I think I saw a comment by David saying that we don't require at least one of these on the server side.

[mjameswh]

That's right, but the schedule is then a no-op. Seems like a reasonable DX addition...

[bergundy]

Still not liking the inconsistency here with the rest of the SDK.
I'm not blocking on this but it deserves a short discussion IMHO.

[lorensr]

Here's the proposal discussion: temporalio/proposals#62 (comment)

[mjameswh]

The only thing that bothers me with reexporting the proto's enum directly is that it doesn't provide type doc. The values in this enum are not trivial to understand for users, so that doc is particularly important.