feat(root): refactor control compilation #7590

djabarovgeorge · 2025-01-27T08:49:09Z

What changed? Why was the change needed?

Screenshots

Expand for optional sections

Related enterprise PR

Special notes for your reviewer

linear · 2025-01-27T08:49:11Z

NV-5076 🐛 Fix rendering of lost variables during Liquid parsing

netlify · 2025-01-27T08:49:25Z

✅ Deploy Preview for dashboard-v2-novu-staging ready!

Name	Link
🔨 Latest commit	`0bb7387`
🔍 Latest deploy log	https://app.netlify.com/sites/dashboard-v2-novu-staging/deploys/6797666df96612000895145b
😎 Deploy Preview	https://deploy-preview-7590.dashboard-v2.novu-staging.co
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify · 2025-01-27T08:49:29Z

✅ Deploy Preview for dev-web-novu ready!

Name	Link
🔨 Latest commit	`0bb7387`
🔍 Latest deploy log	https://app.netlify.com/sites/dev-web-novu/deploys/6797666d040dad00081836cd
😎 Deploy Preview	https://deploy-preview-7590.dashboard.novu-staging.co
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

pkg-pr-new · 2025-01-27T10:30:02Z

Open in Stackblitz

npm i https://pkg.pr.new/novuhq/novu/@novu/framework@7590

npm i https://pkg.pr.new/novuhq/novu@7590

commit: 0bb7387

packages/framework/src/client.ts

djabarovgeorge · 2025-01-27T10:44:40Z

packages/framework/src/client.ts

+        const templateControls = await this.createStepControls(step, event);
+
+        if (this.compileControls) {
+          controls = await this.renderTemplateControls(templateControls, event);


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.

djabarovgeorge · 2025-01-27T10:46:48Z

packages/framework/src/types/config.types.ts

+   * Whether to compile controls.
+   * Defaults to true.
+   */
+  compileControls?: boolean;


Currently, this is a global option. However, we could make it more granular in the future, for example, configurable on a per-step basis.

djabarovgeorge · 2025-01-27T10:52:01Z

apps/api/src/app/environments-v1/usecases/output-renderers/email-output-renderer.usecase.ts

@@ -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);


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.

Is the compileControls flag required for this improvement?

This change is driven by two main reasons:

Performance Improvement: The most obvious benefit is improving performance, as we no longer compile multiple times.

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.

djabarovgeorge · 2025-01-27T10:54:52Z

packages/framework/src/client.ts


      // 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);


This change removes the second (for all client users) or third (for email Novu users) duplicate control compilation, simplifying the process.

scopsy

Looks good, would love to hear some other thoughts on the compileControls terminology 🙏

scopsy · 2025-01-27T11:03:59Z

apps/api/src/app/shared/helpers/liquid.ts

+  return JSON.parse(renderedString);
+}
+
+export async function parseLiquidString(value: string, variables: object): Promise<string> {


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.

good point, i thought about it as well, we need to find a good way to export it from @novu/framework.

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

packages/framework/src/client.ts

SokratisVidros · 2025-02-05T17:24:02Z

apps/api/src/app/environments-v1/usecases/output-renderers/email-output-renderer.usecase.ts

@@ -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);


Is the compileControls flag required for this improvement?

SokratisVidros · 2025-02-05T17:26:18Z

apps/api/src/app/shared/helpers/liquid.ts

+  return JSON.parse(renderedString);
+}
+
+export async function parseLiquidString(value: string, variables: object): Promise<string> {


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

packages/framework/src/client.ts

feat(root): wip refactor control compilation

b912db6

djabarovgeorge temporarily deployed to Linting January 27, 2025 08:49 — with GitHub Actions Inactive

github-actions bot added @novu/dashboard @novu/api-service labels Jan 27, 2025

feat(api-service): add compile flag

b4dd38c

djabarovgeorge temporarily deployed to Linting January 27, 2025 10:27 — with GitHub Actions Inactive

github-actions bot removed the @novu/dashboard label Jan 27, 2025

djabarovgeorge commented Jan 27, 2025

View reviewed changes

packages/framework/src/client.ts Outdated Show resolved Hide resolved

djabarovgeorge commented Jan 27, 2025

View reviewed changes

chore(framework): remove console logs

0bb7387

djabarovgeorge temporarily deployed to Linting January 27, 2025 10:57 — with GitHub Actions Inactive

djabarovgeorge marked this pull request as ready for review January 27, 2025 10:58

scopsy approved these changes Jan 27, 2025

View reviewed changes

djabarovgeorge changed the title ~~feat(root): wip refactor control compilation~~ feat(root): refactor control compilation Feb 2, 2025

SokratisVidros requested changes Feb 5, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(root): refactor control compilation #7590

feat(root): refactor control compilation #7590

djabarovgeorge commented Jan 27, 2025

linear bot commented Jan 27, 2025

netlify bot commented Jan 27, 2025 •

edited

Loading

netlify bot commented Jan 27, 2025 •

edited

Loading

pkg-pr-new bot commented Jan 27, 2025 •

edited

Loading

djabarovgeorge Jan 27, 2025

djabarovgeorge Jan 27, 2025

djabarovgeorge Jan 27, 2025

SokratisVidros Feb 5, 2025

djabarovgeorge Feb 5, 2025

djabarovgeorge Jan 27, 2025

scopsy left a comment

scopsy Jan 27, 2025

djabarovgeorge Jan 27, 2025

SokratisVidros Feb 5, 2025

SokratisVidros Feb 5, 2025

SokratisVidros Feb 5, 2025

feat(root): refactor control compilation #7590

Are you sure you want to change the base?

feat(root): refactor control compilation #7590

Conversation

djabarovgeorge commented Jan 27, 2025

What changed? Why was the change needed?

Screenshots

Related enterprise PR

Special notes for your reviewer

linear bot commented Jan 27, 2025

netlify bot commented Jan 27, 2025 • edited Loading

✅ Deploy Preview for dashboard-v2-novu-staging ready!

netlify bot commented Jan 27, 2025 • edited Loading

✅ Deploy Preview for dev-web-novu ready!

pkg-pr-new bot commented Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Main purpose of this PR:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scopsy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netlify bot commented Jan 27, 2025 •

edited

Loading

netlify bot commented Jan 27, 2025 •

edited

Loading

pkg-pr-new bot commented Jan 27, 2025 •

edited

Loading