dustjs icon indicating copy to clipboard operation
dustjs copied to clipboard

dust.onLoad is bad design

Open fsoikin opened this issue 10 years ago • 7 comments

If my application has multiple Dust consumers, which are independent of each other and want to have different onLoad implementations, there is no good way to satisfy that.

Because partial names can be dynamic, their resolution cannot happen during compile phase, and therefore, I cannot wrap the compile call with set-and-restore-onLoad call.

And because rendering is asynchronous, I cannot wrap render call either.

fsoikin avatar Apr 10 '14 16:04 fsoikin

Can you describe the use case for multiple onload methods?

jimmyhchan avatar Apr 10 '14 16:04 jimmyhchan

Currently, I do not have a real use case. I have noticed this issue as soon as I tried to use onLoad in my code. Just smells bad.

But I do have a possible use case.

We have built a TypeScript translator tool that uses dustjs for generating translation result. It is perfectly reasonable that this tool can be used in an Express application, which also happens to use dustjs for HTML rendering. The TsT tool would then have one implementation of onLoad (loading templates from wherever templates are, appending a ".tpl" extension), and the HTML-rendering part of the app would have a different implementation (loading templates from where the HTML is, appending the ".html" extension).

Good enough?

fsoikin avatar Apr 10 '14 16:04 fsoikin

Thanks. This is helpful. I agree this, like the helpers, is badly designed.

Do you feel you need a clean way to replace the onload or a way to register/de register multiple methods. That is, is simply overwriting the method an option?

jimmyhchan avatar Apr 10 '14 17:04 jimmyhchan

I think the "good" and "right" solution would be to allow for creating multiple instances of the whole Dust engine, while retaining one "default" instance for backwards compatibility - much like RequireJS does.

But I don't mean to say this is critical or urgent to me in any way right now. I just noticed this inconsistency and thought I'd report it.

fsoikin avatar Apr 10 '14 17:04 fsoikin

I wrote something that's been very pleasant in dust 2.6 -- dustjacket, which replaces load and onload with a pluggable implementation.

A major use for pluggable onload is to handle internationalizing templates, where varying the template that's used depending on contextual information is super useful, rather than having a 1:1 name to template mapping.

aredridel avatar Apr 20 '15 17:04 aredridel

I am also working on the same, where i need to fetch templates from database as well as from filesystem at the same time. Created xdust package (WIP) to perform so. Also, working on a centralized caching system for dust in a scalable NodeJS application, where i need to clear specific dust cache based on event generated on a single NodeJS application thread.

ajaysinghj8 avatar Apr 01 '16 18:04 ajaysinghj8

Here we are, 2017, and the problem hasn't gone away.

Consider the following - components, each with their own render method, being required by the main app.

const dust = require('dustjs-helpers');

dust.onLoad = function(name, options, callback) {
    const tmpl = require(`./templates/${name}.dust.html`);
    callback(null, tmpl);
};

export function render(context) {
    return new Promise((resolve, reject) => {
        dust.render('index', context, (err, markup) => {
            if (!err) {
                resolve(markup);
            } else {
                reject(err);
            }
        });
    });
};

Clearly, onLoad must then be shared among all components that use dust, even though some of them may want to load templates lazily or even construct them dynamically based on user data or what have you.

There is a simple solution, where onLoad is registered on context, allowing separate onLoads for separate dust.render calls.

It can be done, without any changes, even today, to an extent!

Somewhere, at the root of the app, register a generic onLoad:

dust.onLoad = function(name, options, callback) {
    const {onLoad} = options;
    onLoad(name, callback);
};

Then, in the component, provide the onLoad:

function customOnLoad(name, callback) {
    const tmpl = require(`./templates/${name}.dust.html`);
    callback(null, tmpl);
}

export function render(context) {
    const augmentedContext = dust.context(context, {
        onLoad: customOnLoad
    });

    return new Promise((resolve, reject) => {
        dust.render('index', augmentedContext, (err, markup) => {
            if (!err) {
                resolve(markup);
            } else {
                reject(err);
            }
        });
    });
};


This, of course, means that you can't use "default" onLoad (which is to say -- there is none, so some of the code in dust.js doesn't run) and you must always provide onLoad on all contexts.

Should cover my case above, however.

dnutels avatar Jan 24 '17 14:01 dnutels