feat(root): wip refactor control compilation #7590
Conversation
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
✅ Deploy Preview for dashboard-v2-novu-staging ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for dev-web-novu ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
commit: |
| const templateControls = await this.createStepControls(step, event); | ||
|
|
||
| if (this.compileControls) { | ||
| controls = await this.renderTemplateControls(templateControls, event); |
There was a problem hiding this comment.
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.
| * Whether to compile controls. | ||
| * Defaults to true. | ||
| */ | ||
| compileControls?: boolean; |
There was a problem hiding this comment.
Currently, this is a global option. However, we could make it more granular in the future, for example, configurable on a per-step basis.
| @@ -33,7 +34,7 @@ export class EmailOutputRendererUsecase { | |||
|
|
|||
| const liquifiedMaily = this.wrapMailyInLiquidUsecase.execute({ emailEditor: body }); | |||
| const transformedMaily = await this.transformMailyContent(liquifiedMaily, renderCommand.fullPayloadForRender); | |||
| const parsedMaily = await this.parseMailyContentByLiquid(transformedMaily, renderCommand.fullPayloadForRender); | |||
| const parsedMaily = await parseLiquid(transformedMaily, renderCommand.fullPayloadForRender); | |||
There was a problem hiding this comment.
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.
|
|
||
| // 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.
This change removes the second (for all client users) or third (for email Novu users) duplicate control compilation, simplifying the process.
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
djabarovgeorge
temporarily deployed
to
Linting
GitHub Actions
Inactive
| return JSON.parse(renderedString); | ||
| } | ||
|
|
||
| export async function parseLiquidString(value: string, variables: object): Promise<string> { |
There was a problem hiding this comment.
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.
good point, i thought about it as well, we need to find a good way to export it from @novu/framework.
| @@ -86,6 +89,7 @@ export class Client { | |||
| apiUrl: resolveApiUrl(providedOptions?.apiUrl), | |||
| secretKey: resolveSecretKey(providedOptions?.secretKey), | |||
| strictAuthentication: !isRuntimeInDevelopment(), | |||
| compileControls: providedOptions?.compileControls ?? true, | |||
There was a problem hiding this comment.
I wonder if compileControls is the most explaining terminology of what actually happens. As Compile can mean a lot of things 🤔 should we be more direct here: parseLiquidControls or smth? I do remember that we wanted to allow to replace the liquid with other custom parsers tho
There was a problem hiding this comment.
Valid point. This was the most fragile change in the i n this PR. As we think of the future evolution of this package, it’s essential to design it properly.
I wouldn’t go with parseLiquidControls, as it’s too specific. As you mentioned, other libraries could handle control compilation, so we need a more generic approach.
What changed? Why was the change needed?
Screenshots
Expand for optional sections
Related enterprise PR
Special notes for your reviewer