-
Notifications
You must be signed in to change notification settings - Fork 895
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
feat: piece branching #4599
feat: piece branching #4599
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 138bd25. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 3 targetsSent with 💌 from NxCloud. |
packages/pieces/community/approval/src/lib/actions/wait-for-approval.ts
Outdated
Show resolved
Hide resolved
…s/activepieces into feat/piece-branching
…w trigger or action
packages/pieces/community/approval/src/lib/actions/wait-for-approval.ts
Outdated
Show resolved
Hide resolved
packages/server/api/src/app/flows/flow/approval/approval.controller.ts
Outdated
Show resolved
Hide resolved
packages/server/api/src/app/flows/flow/approval/approval.service.ts
Outdated
Show resolved
Hide resolved
}, | ||
}, | ||
// Here we define the names of the outputs expected to be branched from the piece | ||
outputs: [ |
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.
Instead of using a name since you are using it as an id
We can change this to an object where the key is the name of the branch
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 the issue would be mainly with adding other properties to the branch in case we would need to add this later, if we would go with an object, I suggest this way:
{
branchName: {
expectedOutput: true,
...otherProperties,
},
}
by this way, we would be able to extend the functionality further in the future. What do you think?
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.
map of objects work
}, | ||
}, | ||
// Here we define the names of the outputs expected to be branched from the piece | ||
outputs: [ |
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.
map of objects work
@@ -58,6 +58,9 @@ export const PieceActionSettings = Type.Object({ | |||
actionName: Type.Optional(Type.String({})), | |||
input: Type.Record(Type.String({}), Type.Any()), | |||
inputUiInfo: SampleDataSettingsObject, | |||
outputs: Type.Optional(Type.Array(Type.Object({ |
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.
@@ -190,6 +193,10 @@ export const Action = Type.Recursive(action => Type.Union([ | |||
})]), | |||
Type.Intersect([PieceActionSchema, Type.Object({ | |||
nextAction: Type.Optional(action), | |||
children: Type.Record( | |||
Type.String({}), | |||
Type.Union([action, Type.Undefined()]), |
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.
action
as value should be enough
@@ -65,6 +66,23 @@ function deleteAction( | |||
parentStep.nextAction = stepToUpdate.nextAction | |||
} | |||
switch (parentStep.type) { | |||
case ActionType.PIECE: { | |||
if ( |
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.
The frontend should tell us the branch name (which is the same as what is defined in the outputs in the piece), and we should update the action in children map. We should never hardcode 'onFailureAction' and 'onSuccessAction'.
@@ -349,6 +403,22 @@ function updateAction( | |||
parentStep.firstLoopAction = createAction(request, actions) | |||
} | |||
} | |||
if (isPieceBranched(parentStep)) { |
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.
Same comment as above this is hardcoded.
|
||
return branchedPieceResponse() | ||
} else { | ||
return branchedPieceResponse({ |
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 are depending on the value to decide where to branch this is wrong, what if both were true.
What if i want to include information like it were approved by who? I think you should use key as to execute branch or not this can be rewritten like that
async function runBranchablePieceWithVersion({
pieceOutput,
executionState,
action,
constants,
stepOutput,
version = 'v1',
}: RunBranchablePieceWithVersion): Promise<FlowExecutorContext> {
const versions = {
v1: async (): Promise<FlowExecutorContext> => {
let newExecutionContext = executionState
for (const [branchKey, _branchValue] of pieceOutput.output.entries()) {
const childAction = action.children[branchKey]
if (isNil(childAction)) {
continue
}
newExecutionContext = await flowExecutor.execute({
action: childAction,
executionState: newExecutionContext,
constants,
})
break;
}
return newExecutionContext.upsertStep(action.name, stepOutput
.setOutput(pieceOutput.output))
.increaseTask()
.setVerdict(ExecutionVerdict.RUNNING, undefined)
},
}
return versions[version]()
}
Also you can change generate approval link to
async run(ctx) {
return {
approveLink: ctx.generateResumeUrl({
queryParams: { action: 'approve' },
}),
denyLink: ctx.generateResumeUrl({
queryParams: { action: 'deny' },
}),
};
```
import { createAction } from '@activepieces/pieces-framework'; | ||
import { ExecutionType, PauseType, branchedPieceResponse } from '@activepieces/shared'; | ||
|
||
export const waitForApprovalLink = createAction({ |
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 will also have to bump package.json
@@ -415,6 +415,7 @@ export class PieceInputFormComponent extends InputFormCore { | |||
...selectedTriggerOrAction.props, | |||
...spreadIfDefined(AUTHENTICATION_PROPERTY_NAME, authProperty), | |||
}, | |||
outputs: selectedTriggerOrAction.outputs, |
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.
This has to be removed, It's duplicated information, you already have it in definition selectedTriggerOrAction
@@ -383,6 +383,7 @@ export class StepTypeSidebarComponent implements AfterViewInit { | |||
pieceVersion: | |||
flowItemDetails.extra?.pieceVersion ?? 'NO_APP_VERSION', | |||
actionName: suggestion?.name, | |||
outputs: suggestion?.outputs, |
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.
This has to be removed, It's duplicated information, you already have it in definition suggestion.outputs
if (step.type === ActionType.BRANCH) { | ||
return index === 0 ? $localize`True` : $localize`False`; | ||
} else { | ||
return index === 0 |
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 already allow array in the framework, this will not work for the documentation you have wrote.
The children array / outputs array in the piece can be longer than 2 elements
What does this PR do?
Fixes #2065