-
Notifications
You must be signed in to change notification settings - Fork 4k
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(root): refactor control compilation #7590
base: next
Are you sure you want to change the base?
Changes from all 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 |
---|---|---|
@@ -1,6 +1,13 @@ | ||
import { Liquid } from 'liquidjs'; | ||
|
||
export const parseLiquid = async (value: string, variables: object): Promise<string> => { | ||
export async function parseLiquid<T>(value: T, variables: object): Promise<T> { | ||
const valueStringified = JSON.stringify(value); | ||
const renderedString = await parseLiquidString(valueStringified, variables); | ||
|
||
return JSON.parse(renderedString); | ||
} | ||
|
||
export async function parseLiquidString(value: string, variables: object): Promise<string> { | ||
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. A small question, is it possible some how to reuse the same liquid instance from the framework SDK? The reason I'm asking is because we might want to add custom liquid filters to the system, and in this case we will have to duplicate them twice. 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. good point, i thought about it as well, we need to find a good way to export it from @novu/framework. 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. We can export it from |
||
const client = new Liquid({ | ||
outputEscape: (output) => { | ||
return stringifyDataStructureWithSingleQuotes(output); | ||
|
@@ -10,7 +17,7 @@ export const parseLiquid = async (value: string, variables: object): Promise<str | |
const template = client.parse(value); | ||
|
||
return await client.render(template, variables); | ||
}; | ||
} | ||
|
||
const stringifyDataStructureWithSingleQuotes = (value: unknown, spaces: number = 0): string => { | ||
if (Array.isArray(value) || (typeof value === 'object' && value !== null)) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -71,11 +71,14 @@ export class Client { | |
|
||
public strictAuthentication: boolean; | ||
|
||
public compileControls: boolean; | ||
|
||
constructor(options?: ClientOptions) { | ||
const builtOpts = this.buildOptions(options); | ||
this.apiUrl = builtOpts.apiUrl; | ||
this.secretKey = builtOpts.secretKey; | ||
this.strictAuthentication = builtOpts.strictAuthentication; | ||
this.compileControls = builtOpts.compileControls; | ||
this.templateEngine.registerFilter('json', (value, spaces) => | ||
stringifyDataStructureWithSingleQuotes(value, spaces) | ||
); | ||
|
@@ -86,6 +89,7 @@ export class Client { | |
apiUrl: resolveApiUrl(providedOptions?.apiUrl), | ||
secretKey: resolveSecretKey(providedOptions?.secretKey), | ||
strictAuthentication: !isRuntimeInDevelopment(), | ||
compileControls: providedOptions?.compileControls ?? true, | ||
djabarovgeorge marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}; | ||
|
||
if (providedOptions?.strictAuthentication !== undefined) { | ||
|
@@ -309,11 +313,20 @@ export class Client { | |
|
||
const step = this.getStep(event.workflowId, stepId); | ||
const isPreview = event.action === PostActionEnum.PREVIEW; | ||
let controls = {}; | ||
|
||
if (stepId === event.stepId) { | ||
const templateControls = await this.createStepControls(step, event); | ||
|
||
if (this.compileControls) { | ||
controls = await this.renderTemplateControls(templateControls, 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. Currently, the framework compiles the controls by default. However, if the user prefers to compile the controls themselves, they have the option to do so. |
||
} else { | ||
controls = templateControls; | ||
} | ||
} | ||
|
||
// Only evaluate a skip condition when the step is the current step and not in preview mode. | ||
if (!isPreview && stepId === event.stepId) { | ||
const templateControls = await this.createStepControls(step, event); | ||
const controls = await this.compileControls(templateControls, 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. This change removes the second (for all client users) or third (for email Novu users) duplicate control compilation, simplifying the process. |
||
const shouldSkip = await this.shouldSkip(options?.skip as typeof step.options.skip, controls); | ||
|
||
if (shouldSkip) { | ||
|
@@ -336,24 +349,28 @@ export class Client { | |
const executeStepHandler = this.executeStep.bind(this); | ||
const handler = isPreview ? previewStepHandler : executeStepHandler; | ||
|
||
let stepResult = await handler(event, { | ||
...step, | ||
providers: step.providers.map((provider) => { | ||
// TODO: Update return type to include ChannelStep and fix typings | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
const providerResolve = (options as any)?.providers?.[provider.type] as typeof provider.resolve; | ||
|
||
if (!providerResolve) { | ||
throw new ProviderNotFoundError(provider.type); | ||
} | ||
|
||
return { | ||
...provider, | ||
resolve: providerResolve, | ||
}; | ||
}), | ||
resolve: stepResolve as typeof step.resolve, | ||
}); | ||
let stepResult = await handler( | ||
event, | ||
{ | ||
...step, | ||
providers: step.providers.map((provider) => { | ||
// TODO: Update return type to include ChannelStep and fix typings | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
const providerResolve = (options as any)?.providers?.[provider.type] as typeof provider.resolve; | ||
|
||
if (!providerResolve) { | ||
throw new ProviderNotFoundError(provider.type); | ||
} | ||
|
||
return { | ||
...provider, | ||
resolve: providerResolve, | ||
}; | ||
}), | ||
resolve: stepResolve as typeof step.resolve, | ||
}, | ||
controls | ||
); | ||
|
||
if ( | ||
Object.values(ChannelStepEnum).includes(step.type as ChannelStepEnum) && | ||
|
@@ -634,12 +651,11 @@ export class Client { | |
|
||
private async executeStep( | ||
event: Event, | ||
step: DiscoverStepOutput | ||
step: DiscoverStepOutput, | ||
controls: Record<string, unknown> | ||
): Promise<Pick<ExecuteOutput, 'outputs' | 'providers'>> { | ||
if (event.stepId === step.stepId) { | ||
try { | ||
const templateControls = await this.createStepControls(step, event); | ||
const controls = await this.compileControls(templateControls, event); | ||
const output = await step.resolve(controls); | ||
const validatedOutput = await this.validate( | ||
output, | ||
|
@@ -696,7 +712,7 @@ export class Client { | |
} | ||
} | ||
|
||
private async compileControls(templateControls: Record<string, unknown>, event: Event) { | ||
private async renderTemplateControls(templateControls: Record<string, unknown>, event: Event) { | ||
try { | ||
const templateString = this.templateEngine.parse(JSON.stringify(templateControls)); | ||
|
||
|
@@ -734,10 +750,11 @@ export class Client { | |
|
||
private async previewStep( | ||
event: Event, | ||
step: DiscoverStepOutput | ||
step: DiscoverStepOutput, | ||
controls: Record<string, unknown> | ||
): Promise<Pick<ExecuteOutput, 'outputs' | 'providers'>> { | ||
try { | ||
return await this.constructStepForPreview(event, step); | ||
return await this.constructStepForPreview(event, step, controls); | ||
} catch (error) { | ||
console.log(` ${EMOJI.ERROR} Failed to preview stepId: \`${step.stepId}\``); | ||
|
||
|
@@ -749,9 +766,9 @@ export class Client { | |
} | ||
} | ||
|
||
private async constructStepForPreview(event: Event, step: DiscoverStepOutput) { | ||
private async constructStepForPreview(event: Event, step: DiscoverStepOutput, controls: Record<string, unknown>) { | ||
if (event.stepId === step.stepId) { | ||
return await this.previewRequiredStep(step, event); | ||
return await this.previewRequiredStep(step, event, controls); | ||
} else { | ||
return await this.extractMockDataForPreviousSteps(event, step); | ||
} | ||
|
@@ -770,10 +787,7 @@ export class Client { | |
}; | ||
} | ||
|
||
private async previewRequiredStep(step: DiscoverStepOutput, event: Event) { | ||
const templateControls = await this.createStepControls(step, event); | ||
const controls = await this.compileControls(templateControls, event); | ||
|
||
private async previewRequiredStep(step: DiscoverStepOutput, event: Event, controls: Record<string, unknown>) { | ||
const previewOutput = await step.resolve(controls); | ||
const validatedOutput = await this.validate( | ||
previewOutput, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,4 +23,10 @@ export type ClientOptions = { | |
* Defaults to true. | ||
*/ | ||
strictAuthentication?: boolean; | ||
|
||
/** | ||
* Whether to compile controls. | ||
* Defaults to true. | ||
*/ | ||
compileControls?: boolean; | ||
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. Currently, this is a global option. However, we could make it more granular in the future, for example, configurable on a per-step basis. |
||
}; |
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.
Main purpose of this PR:
Previously, the framework would parse controls once before execution, and then parse them again during execution.
With this update, controls are parsed only once, improving efficiency.
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.
Is the
compileControls
flag required for this improvement?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 change is driven by two main reasons:
From the beginning, we relied on
maily.variable.type
, but there came a point when we considered decoupling frommaily.variable.type
and switching tomaily.text.type
using the simple text with{{...}}
syntax. This approach would allow us to manage variables independently using Liquid, a change I support because it grants us greater control and simplifies our codebase.Why is this important? If we do not control the content rendering process and the framework renders the controls before we execute the "render" step, it could lead to bugs like the one mentioned. Taking control of the rendering process helps prevent such issues.