-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
fix(framework): Stop validating controls for non previewed step #6876
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,7 @@ import type { | |
HealthCheck, | ||
Schema, | ||
Skip, | ||
State, | ||
rifont marked this conversation as resolved.
Show resolved
Hide resolved
|
||
ValidationError, | ||
Workflow, | ||
} from './types'; | ||
|
@@ -277,25 +278,27 @@ export class Client { | |
} | ||
|
||
const step = this.getStep(event.workflowId, stepId); | ||
const controls = await this.createStepControls(step, event); | ||
const isPreview = event.action === PostActionEnum.PREVIEW; | ||
|
||
if (!isPreview && (await this.shouldSkip(options?.skip as typeof step.options.skip, controls))) { | ||
if (stepId === event.stepId) { | ||
// Only set the result when the step is the current step. | ||
// Only evaluate a skip condition when the step is the current step and not in preview mode. | ||
if (!isPreview && stepId === event.stepId) { | ||
const controls = await this.createStepControls(step, event); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With this change, we ensure step controls are only validated during execution where the controls are guaranteed to be available for the current step being executed. |
||
const shouldSkip = await this.shouldSkip(options?.skip as typeof step.options.skip, controls); | ||
|
||
if (shouldSkip) { | ||
setResult({ | ||
options: { skip: true }, | ||
outputs: {}, | ||
providers: {}, | ||
}); | ||
} | ||
|
||
/* | ||
* Return an empty object for results when a step is skipped. | ||
* TODO: fix typings when `skip` is specified to return `Partial<T_Result>` | ||
*/ | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
return {} as any; | ||
/* | ||
* Return an empty object for results when a step is skipped. | ||
* TODO: fix typings when `skip` is specified to return `Partial<T_Result>` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just to make sure it doesn't bypass the second PR and results will be supplied if i provide them There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this code branch is only for the non-preview case. |
||
*/ | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
return {} as any; | ||
} | ||
} | ||
|
||
const previewStepHandler = this.previewStep.bind(this); | ||
|
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.
An empty
outputs
should have been supplied for this test, ensuring that a hydration can still occur for skipped steps. This fixes the assertion.