Make Liquidjs RenderOptions available

[jod3]

Describe the Improvement

We currently have a case where we want to render a liquid template with other RenderOptions than the defaults.

Proposal: Forward access to the RenderOptions in MailService.renderTemplate() and MailService.send()

https://github.com/directus/directus/blob/main/api/src/services/mail/index.ts#L27

export type EmailOptions = SendMailOptions & {
	template?: {
		name: string;
		data: Record<string, any>;
		renderOptions: RenderOptions
	};
};

https://github.com/directus/directus/blob/main/api/src/services/mail/index.ts#L81

html = await this.renderTemplate(template.name, templateData, options?.template?.renderOptions || {} );

https://github.com/directus/directus/blob/main/api/src/services/mail/index.ts#L96

async renderTemplate(template, variables, renderOptions) {
        const customTemplatePath = path.resolve(env['EMAIL_TEMPLATES_PATH'], template + '.liquid');
        const systemTemplatePath = path.join(__dirname, 'templates', template + '.liquid');
        const templatePath = (await fse.pathExists(customTemplatePath)) ? customTemplatePath : systemTemplatePath;
        if ((await fse.pathExists(templatePath)) === false) {
            throw new InvalidPayloadError({ reason: `Template "${template}" doesn't exist` });
        }
        const templateString = await fse.readFile(templatePath, 'utf8');
        const html = await liquidEngine.parseAndRender(templateString, variables, renderOptions);
        return html;
    }

[rijkvanzanten]

What kind of render options are you looking to override?

[jod3]

In our specific case we want to make the template rendering fail if the a value is not set int the scope.

see https://github.com/harttle/liquidjs/blob/542a75fd44eecdeac8e5ff9ab19ed334c194d86c/src/liquid-options.ts#L88 and strictVariables

[rijkvanzanten]

@jod3 That the only one? Seeing that the liquid options are pretty tied to server integration and performance¹, I'm wondering if it makes more sense to just set that particular option globally across the board. Feels like a safe one to have regardless!

Footnotes

1. Things like sync vs async really shouldn't be changed on a case by case basis ↩

[jod3]

1. Things like sync vs async really shouldn't be changed on a case by case basis ↩

Why do you think so? This whole issue is pretty much developer focused, so for sb. who wants to leave the basics and develop extensions living in directus. If it makes sense for a specific extension to render the template async it should not affect directus basic or impact the server (more than sync could do)? Or am I missing sth.

I'm wondering if it makes more sense to just set that particular option globally across the board. Feels like a safe one to have regardless!

There might be good reasons to fallback to an empty string if a value is not set. Alternative is throwing an exceptions on missing values which might raise issues out there.

[rijkvanzanten]

Why do you think so? This whole issue is pretty much developer focused, so for sb. who wants to leave the basics and develop extensions living in directus. If it makes sense for a specific extension to render the template async it should not affect directus basic or impact the server (more than sync could do)? Or am I missing sth.

Those extensions run within the context of the Directus server itself, so if you execute a long running sync function you will indeed affect the Directus api as a whole.

Generally speaking exposing all options from a third party library like this means that we're tying ourselves to this library. We've shot ourselves in the foot with this in the past with things like keyv vs redis, where switching the underlying logic executing the behavior needs to be changed or swapped out, but because we supported the full public API of the internal APIs used we now either have to ship a big breaking change, or reimplement support for the previous library's options in the new one.

I don't think liquidjs will go anywhere anytime soon, but at the same time I have to plan for long term stable use and options, which means I have to pay attention to what we have to / want to support rather than blanketly opening up everything and potentially cause issues later 🙂