-
Notifications
You must be signed in to change notification settings - Fork 109
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
Workflow Update #1277
Workflow Update #1277
Conversation
// expiry. See https://github.com/temporalio/temporal/issues/4742 | ||
|
||
// TODO: When temporal#4742 is released, stop catching DEADLINE_EXCEEDED. | ||
while (true) { |
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.
May also want a TODO concerning supporting user-facing cancellation on long-poll/RPC-loop calls (AbortSignal
I think was what was decided) and a link to the issue (or make one)
protected async _executeUpdateHandler<Ret>(input: WorkflowUpdateInput): Promise<Ret> { | ||
const handle = await this._startUpdate<Ret>( | ||
input, | ||
temporal.api.enums.v1.UpdateWorkflowExecutionLifecycleStage.UPDATE_WORKFLOW_EXECUTION_LIFECYCLE_STAGE_COMPLETED |
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 might want to have a type alias here to avoid this long statement.
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.
Yay! 💃
d897c74
to
1dbba9b
Compare
9a11f66
to
2ddfae5
Compare
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.
I think we agreed to mark all of the update APIs experimental, please do so.
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.
Meant to comment before, accidentally approved.
e970ab5
to
8248aad
Compare
In addition to the tests here, this PR passes the features tests for TS in temporalio/features#362 |
This reverts commit 5aad81b.
if (!name) { | ||
throw new TypeError('Missing activation update name'); | ||
} | ||
if (!protocolInstanceId) { |
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.
nit: You could have combine all of those checks into a single "Misformed activation" error. Being very detailed is useful for user provided objects (ie. errors are more likely, are user need to know what they need to fix), but here, a missing field would indicate a server/core level error.
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.
Please make sure to fix the four floating promises before you merge. Otherwise, just a suggestion, don't feel obliged.
Great job, thanks a lot!
d531d80
to
0477861
Compare
0b487bc
to
26c2e9d
Compare
Pure refactoring of the integration tests, done in preparation for implementation of Workflow Update in temporalio#1277.
Fixes #1160
API
To see examples using the new API, see
packages/test/src/test-integration-update-*
Briefly:
How was this tested
packages/test/src/integration-tests/test-update*
Checklist
Closes [Typescript] Workflow update support #1160
Any docs updates needed?
Yes
Not done in this PR:
firstExecutionRunId
)