Skip to content
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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: next
Choose a base branch
from
Open

feat(root): refactor control compilation #7590

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b912db6
feat(root): wip refactor control compilation
djabarovgeorge Jan 27, 2025
b4dd38c
feat(api-service): add compile flag
djabarovgeorge Jan 27, 2025
0bb7387
chore(framework): remove console logs
djabarovgeorge Jan 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/api/src/app/environments-v1/novu-bridge-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export class NovuBridgeClient {
this.novuRequestHandler = new NovuRequestHandler({
frameworkName,
workflows,
client: new Client({ secretKey, strictAuthentication: true }),
client: new Client({ secretKey, strictAuthentication: true, compileControls: false }),
handler: this.novuHandler.handler,
});

Expand Down
5 changes: 3 additions & 2 deletions ...ronments-v1/usecases/construct-framework-workflow/construct-framework-workflow.usecase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { DelayOutputRendererUsecase } from '../output-renderers/delay-output-ren
import { DigestOutputRendererUsecase } from '../output-renderers/digest-output-renderer.usecase';
import { evaluateRules } from '../../../shared/services/query-parser/query-parser.service';
import { isMatchingJsonSchema } from '../../../workflows-v2/util/jsonToSchema';
import { parseLiquid, parseLiquidString } from '../../../shared/helpers/liquid';

const LOG_CONTEXT = 'ConstructFrameworkWorkflow';

Expand Down Expand Up @@ -239,8 +240,8 @@ export class ConstructFrameworkWorkflow {
if (_.isEmpty(skipRules)) {
return false;
}

const { result, error } = evaluateRules(skipRules, variables);
const compiledSkipRules = await parseLiquid(skipRules, variables);
const { result, error } = evaluateRules(compiledSkipRules, variables);

if (error) {
this.logger.error({ err: error }, 'Failed to evaluate skip rule', LOG_CONTEXT);
Expand Down
6 changes: 4 additions & 2 deletions apps/api/src/app/environments-v1/usecases/output-renderers/chat-output-renderer.usecase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ import { ChatRenderOutput } from '@novu/shared';
import { Injectable } from '@nestjs/common';
import { InstrumentUsecase } from '@novu/application-generic';
import { RenderCommand } from './render-command';
import { parseLiquid } from '../../../shared/helpers/liquid';

@Injectable()
export class ChatOutputRendererUsecase {
@InstrumentUsecase()
execute(renderCommand: RenderCommand): ChatRenderOutput {
async execute(renderCommand: RenderCommand): Promise<ChatRenderOutput> {
const { skip, ...outputControls } = renderCommand.controlValues ?? {};
const parsedOutputControls = await parseLiquid(outputControls, renderCommand.fullPayloadForRender);

return outputControls as any;
return parsedOutputControls as any;
}
}
6 changes: 4 additions & 2 deletions apps/api/src/app/environments-v1/usecases/output-renderers/delay-output-renderer.usecase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ import { Injectable } from '@nestjs/common';
import { DelayRenderOutput } from '@novu/shared';
import { InstrumentUsecase } from '@novu/application-generic';
import { RenderCommand } from './render-command';
import { parseLiquid } from '../../../shared/helpers/liquid';

@Injectable()
export class DelayOutputRendererUsecase {
@InstrumentUsecase()
execute(renderCommand: RenderCommand): DelayRenderOutput {
async execute(renderCommand: RenderCommand): Promise<DelayRenderOutput> {
const { skip, ...outputControls } = renderCommand.controlValues ?? {};
const parsedOutputControls = await parseLiquid(outputControls, renderCommand.fullPayloadForRender);

return outputControls as any;
return parsedOutputControls as any;
}
}
6 changes: 4 additions & 2 deletions apps/api/src/app/environments-v1/usecases/output-renderers/digest-output-renderer.usecase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ import { Injectable, InternalServerErrorException } from '@nestjs/common';
import { DigestRenderOutput } from '@novu/shared';
import { InstrumentUsecase } from '@novu/application-generic';
import { RenderCommand } from './render-command';
import { parseLiquid } from '../../../shared/helpers/liquid';

@Injectable()
export class DigestOutputRendererUsecase {
@InstrumentUsecase()
execute(renderCommand: RenderCommand): DigestRenderOutput {
async execute(renderCommand: RenderCommand): Promise<DigestRenderOutput> {
const { skip, ...outputControls } = renderCommand.controlValues ?? {};
const parsedOutputControls = await parseLiquid(outputControls, renderCommand.fullPayloadForRender);

return outputControls as any;
return parsedOutputControls as any;
}
}
22 changes: 7 additions & 15 deletions apps/api/src/app/environments-v1/usecases/output-renderers/email-output-renderer.usecase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { InstrumentUsecase } from '@novu/application-generic';
import { FullPayloadForRender, RenderCommand } from './render-command';
import { WrapMailyInLiquidUseCase } from './maily-to-liquid/wrap-maily-in-liquid.usecase';
import { MAILY_ITERABLE_MARK, MailyAttrsEnum, MailyContentTypeEnum } from './maily-to-liquid/maily.types';
import { parseLiquid } from '../../../shared/helpers/liquid';
import { parseLiquid, parseLiquidString } from '../../../shared/helpers/liquid';

export class EmailOutputRendererCommand extends RenderCommand {}

Expand All @@ -17,7 +17,8 @@ export class EmailOutputRendererUsecase {

@InstrumentUsecase()
async execute(renderCommand: EmailOutputRendererCommand): Promise<EmailRenderOutput> {
const { body, subject } = renderCommand.controlValues;
const { body, subject: controlSubject } = renderCommand.controlValues;
const subject = await parseLiquidString(controlSubject as string, renderCommand.fullPayloadForRender);

if (!body || typeof body !== 'string') {
/**
Expand All @@ -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);
Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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:

  1. Performance Improvement: The most obvious benefit is improving performance, as we no longer compile multiple times.
  2. Flexibility and Bug Prevention: The second reason is increased flexibility and the potential to address bugs, which I will elaborate on.

From the beginning, we relied on maily.variable.type, but there came a point when we considered decoupling from maily.variable.type and switching to maily.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.

const strippedMaily = this.removeTrailingEmptyLines(parsedMaily);
const renderedHtml = await mailyRender(strippedMaily);

Expand All @@ -42,7 +43,7 @@ export class EmailOutputRendererUsecase {
* This passes responsibility to framework to throw type validation exceptions
* rather than handling invalid types here.
*/
return { subject: subject as string, body: renderedHtml };
return { subject, body: renderedHtml };
}

private removeTrailingEmptyLines(node: MailyJSONContent): MailyJSONContent {
Expand All @@ -69,15 +70,6 @@ export class EmailOutputRendererUsecase {
return { ...node, content: filteredContent };
}

private async parseMailyContentByLiquid(
mailyContent: MailyJSONContent,
variables: FullPayloadForRender
): Promise<MailyJSONContent> {
const parsedString = await parseLiquid(JSON.stringify(mailyContent), variables);

return JSON.parse(parsedString);
}

private async transformMailyContent(
node: MailyJSONContent,
variables: FullPayloadForRender,
Expand Down Expand Up @@ -146,7 +138,7 @@ export class EmailOutputRendererUsecase {
node: MailyJSONContent & { attrs: { [MailyAttrsEnum.SHOW_IF_KEY]: string } }
): Promise<boolean> {
const { [MailyAttrsEnum.SHOW_IF_KEY]: showIfKey } = node.attrs;
const parsedShowIfValue = await parseLiquid(showIfKey, variables);
const parsedShowIfValue = await parseLiquidString(showIfKey, variables);

return this.stringToBoolean(parsedShowIfValue);
}
Expand Down Expand Up @@ -209,7 +201,7 @@ export class EmailOutputRendererUsecase {
}

private async getIterableArray(iterablePath: string, variables: FullPayloadForRender): Promise<unknown[]> {
const iterableArrayString = await parseLiquid(iterablePath, variables);
const iterableArrayString = await parseLiquidString(iterablePath, variables);

try {
const parsedArray = JSON.parse(iterableArrayString.replace(/'/g, '"'));
Expand Down
6 changes: 4 additions & 2 deletions apps/api/src/app/environments-v1/usecases/output-renderers/in-app-output-renderer.usecase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ import { InAppRenderOutput } from '@novu/shared';
import { Injectable } from '@nestjs/common';
import { InstrumentUsecase } from '@novu/application-generic';
import { RenderCommand } from './render-command';
import { parseLiquid } from '../../../shared/helpers/liquid';

@Injectable()
export class InAppOutputRendererUsecase {
@InstrumentUsecase()
execute(renderCommand: RenderCommand): InAppRenderOutput {
async execute(renderCommand: RenderCommand): Promise<InAppRenderOutput> {
const { skip, disableOutputSanitization, ...outputControls } = renderCommand.controlValues ?? {};
const parsedOutputControls = await parseLiquid(outputControls, renderCommand.fullPayloadForRender);

return outputControls as any;
return parsedOutputControls as any;
}
}
6 changes: 4 additions & 2 deletions apps/api/src/app/environments-v1/usecases/output-renderers/push-output-renderer.usecase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ import { Injectable } from '@nestjs/common';
import { PushRenderOutput } from '@novu/shared';
import { InstrumentUsecase } from '@novu/application-generic';
import { RenderCommand } from './render-command';
import { parseLiquid } from '../../../shared/helpers/liquid';

@Injectable()
export class PushOutputRendererUsecase {
@InstrumentUsecase()
execute(renderCommand: RenderCommand): PushRenderOutput {
async execute(renderCommand: RenderCommand): Promise<PushRenderOutput> {
const { skip, ...outputControls } = renderCommand.controlValues ?? {};
const parsedOutputControls = await parseLiquid(outputControls, renderCommand.fullPayloadForRender);

return outputControls as any;
return parsedOutputControls as any;
}
}
6 changes: 4 additions & 2 deletions apps/api/src/app/environments-v1/usecases/output-renderers/sms-output-renderer.usecase.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ import { Injectable } from '@nestjs/common';
import { SmsRenderOutput } from '@novu/shared';
import { InstrumentUsecase } from '@novu/application-generic';
import { RenderCommand } from './render-command';
import { parseLiquid } from '../../../shared/helpers/liquid';

@Injectable()
export class SmsOutputRendererUsecase {
@InstrumentUsecase()
execute(renderCommand: RenderCommand): SmsRenderOutput {
async execute(renderCommand: RenderCommand): Promise<SmsRenderOutput> {
const { skip, ...outputControls } = renderCommand.controlValues ?? {};
const parsedOutputControls = await parseLiquid(outputControls, renderCommand.fullPayloadForRender);

return outputControls as any;
return parsedOutputControls as any;
}
}
11 changes: 9 additions & 2 deletions apps/api/src/app/shared/helpers/liquid.ts
View file
Open in desktop
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> {
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can export it from @novu/framework/internals.

const client = new Liquid({
outputEscape: (output) => {
return stringifyDataStructureWithSingleQuotes(output);
Expand All @@ -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)) {
Expand Down
Loading
Loading