Add a `cb(null, compiledTemplate)` signature to `dust.onLoad` #641

sethkinast · 2015-04-20T23:52:44Z

Currently you can either call cb(), in which case Dust will try to load the original name from cache,
or cb(null, src), and Dust will try to compile src and register it under the original name.

Sometimes, you might want to dynamically load one of several templates based on the original name.

This change adds cb(null, compiledTemplate) so you can load any template from cache or the filesystem,
register it manually (or not at all), and render it.

The returned template will NOT be registered under the original name.

sethkinast · 2015-04-21T01:44:18Z

Some thoughts:

Maybe explicitly check for the null return case and short-circuit straight to re-fetching from cache

Does the name need to be configurable? Compiled templates have a templateName property if they get compiled with a name. But if they get compiled with a name, that means they get registered with that name. You could manually override it.

aredridel · 2015-04-21T14:49:03Z

I'd suggest accepting an object with instructions on what to do with the template rather than doing magic on null, if that's a needed feature. And keeps an extensible option open, rather than just choosing a particular action for a particular primitive. {registerTemplate: tmpl} maybe. But I don't know that it's needed. A clean place to expose DIY caching and loading is all one needs to replicate things.

sethkinast · 2015-04-21T20:25:50Z

OK, how do you feel about this meeting your needs for Adaro?

Any of these cases work.

dust.onLoad = function(name, callback) {
  dust.loadSource(dust.compile("some other template", "someOtherName"));
  callback(null, 'someOtherName'); // renders the template called `someOtherName`, which has to be cached already

  var someTemplate = dust.loadSource(dust.compile("some other template"));
  someTemplate.templateName; // null
  someTemplate.templateName = 'foobar';
  callback(null, someTemplate); // renders the template. The name of the template is whatever is attached to the template function-- by default this is the name it was compiled with. This template does not get cached automatically.

  callback(null, 'this is the template'); // existing case

  dust.loadSource(dust.compile('this is the template', name));
  callback(); // existing case
};

sethkinast · 2015-04-21T20:32:18Z

Using the first example should give you the ability to do dynamic lookups on every request for a template and map it to the appropriate localized template. You can cache the localized template so it doesn't get recompiled all the time.

You could also change the internal template name if you really needed to, though this isn't super exciting to me as I feel like you could hide issues.

The only weird thing here is that if you were trying to compile a template that had the same body as an existing template name, the wrong thing happens. But I don't think that's a likely case.

sethkinast · 2015-04-21T20:36:52Z

So for dustjacket the simplified version seems like it should be something like:

dust.onLoad = function(name, callback) {
  var newName = lookup_my_i18n_version(name);
  if(!dust.cache[newName]) {
    var src = loadFromFS(whateverPathTo(newName));
    dust.loadSource(dust.compile(src, newName)); // caches under newName for next time
  }
  callback(null, dust.cache[newName]);
};

aredridel · 2015-04-21T20:37:45Z

I hate mixing the two string cases -- that's just going to lead to errors. I'd say letting the onLoad look up the other template in the cache and return it is much smarter.

If onLoad has access to the cache, and it can bypass it, yay! It's got the raw primitives it needs to do the work.

sethkinast · 2015-04-21T20:38:25Z

That's legit. Will fix it.

sethkinast · 2015-04-21T20:40:15Z

No context access is OK, right? You were just storing in there because you could?

I dislike giving onLoad access to the context.

aredridel · 2015-04-21T20:40:34Z

No, that's actually really needed. That's where i18n information lives.

aredridel · 2015-04-21T20:41:08Z

Why do you dislike it?

aredridel · 2015-04-21T20:42:07Z

(I was actually making a PR against this one to add context passing)

sethkinast · 2015-04-21T20:42:14Z

Context is for templates and shouldn't be accessed by entities external to a template (à la functions that aren't context functions).

aredridel · 2015-04-21T20:43:20Z

Because?

aredridel · 2015-04-21T21:10:04Z

I can see the worry if it would extend the lifetime of context beyond the template, but onLoad is very much within the scope of a single render process.

sethkinast · 2015-04-21T22:03:13Z

We talked about this during the weekly Dust meeting. The general thought is that context is for templates to use, and that request configuration (i.e. locale) is disjoint to that scope of concern. We're thinking on it some more.

sethkinast · 2015-04-22T00:43:39Z

So here's what I've ground out after pondering this for quite awhile.

It's not that you need context, it's that you need a request-scoped set of parameters. Express does a really hideous job of this by smashing template locals in with Express config, and it's super gross.

@jimmyhchan alluded to this by mentioning that if we had a middleware stack for onLoad, it would need to be configurable.

This could mean adding some options as a top-level Context property, maybe?

aredridel · 2015-04-22T00:47:35Z

Yeah, anything that passes through the request context helps.

However, we're also looking at doing this client-side -- though in theory, there, locale selected is a global and doesn't vary. The same mechanism in both places would be pleasant though.

I've always thought of the context as a request-scoped set of parameters.

What do you mean by adding some options?

sethkinast · 2015-04-22T00:55:31Z

Context has a stack and globals currently. Maybe it needs some options that
are not intended for the template to use, but the render request itself.

On Tue, Apr 21, 2015, 5:47 PM Aria Stewart notifications@github.com wrote:

Yeah, anything that passes through the request context helps.

However, we're also looking at doing this client-side -- though in theory,
there, locale selected is a global and doesn't vary. The same mechanism in
both places would be pleasant though.

I've always thought of the context as a request-scoped set of parameters.

What do you mean by adding some options?

—
Reply to this email directly or view it on GitHub
#641 (comment).

aredridel · 2015-04-22T01:01:07Z

That sounds delightful!

sethkinast · 2015-04-22T01:10:17Z

The downside of that is that you then have to pass real contexts to dust.render to make use of them, in the same way you add globals using makeBase.

The upside is that you're not polluting template context with options.

aredridel · 2015-04-22T01:33:54Z

I don't find the pollution terribly problematic -- at least not in ways we're not already dealing with -- but passing a real context with makeBase is already something we do and are familiar with.

aredridel · 2015-04-22T13:52:50Z

One interesting hang-up is the specifics of context.getTemplateName() in and around onLoad, doubly so with partials. When doing internationalization in particular, knowing the name of the template requested is super useful; that can't be just what's passed in render though, because of partials. It gets even dicier with blocks.

sethkinast · 2015-04-23T00:23:21Z

dust.onLoad = function(name, opts, callback) {
  if(opts.lang === 'fr') {
    return callback(null, 'la ' + name);
  }
  if(opts.lang === 'es') {
    return callback(null, 'el ' + name);
  }
  return callback(null, name);
};

app.get('/', function(req, res) {
  dust.stream('Woo', dust.context(null, req.query)).pipe(res);
});

sethkinast · 2015-04-23T00:28:03Z

😟?

aredridel · 2015-04-23T02:41:28Z

This looks pretty good. I'm going to run it through some paces tomorrow -- the problems around partials and blocks get pretty specific, so I'm going to make sure it works!

sethkinast · 2015-04-23T03:04:48Z

Nice, thank you.

On Wed, Apr 22, 2015 at 7:41 PM, Aria Stewart notifications@github.com
wrote:

This looks pretty good. I'm going to run it through some paces tomorrow --
the problems around partials and blocks get pretty specific, so I'm going
to make sure it works!

—
Reply to this email directly or view it on GitHub
#641 (comment).

Seth Kinast
http://sethkinast.com/

sethkinast · 2015-04-23T16:43:50Z

Partials definitely invoke the same loading logic as anything else, along with their partial name. But blocks don't have anything to do with onLoading, no?

aredridel · 2015-04-23T16:48:44Z

Yeah. It's just hard because I want my blocks to know what template they came from so they can look up the right i18n strings to use. I did some funky postprocessing that I think will still apply.

aredridel · 2015-04-23T18:51:15Z

After looking this over thoroughly, I think this satisfies our use cases pretty well.

My only comment is that it would be simpler to write error-free integration with context options if it were always an object, rather than sometimes undefined.

sethkinast · 2015-04-23T23:32:20Z

@jimmyhchan no rush, but I want your blessing before we talk about shipping this

jimmyhchan · 2015-04-24T13:59:08Z

lib/dust.js

@@ -270,18 +308,19 @@
    }
  };

-  function Context(stack, global, blocks, templateName) {
+  function Context(stack, global, options, blocks, templateName) {


nit: why this ordering? IMO global and blocks kinda go together.

To pass the minimum number of params for makeBase. But this is completely
internal so it can be whatever if you like.

sethkinast · 2015-04-28T18:13:03Z

Ended up not being as bad as I thought, and I was able to gracefully use my new getTemplate function. So I just added it to this PR.

sethkinast · 2015-04-28T19:37:09Z

Going to look at pulling this in today since this was @jimmyhchan's main blocker. Will review with @prashn64.

Currently you can either call `cb()`, in which case Dust will try to load the original name from cache, or `cb(null, src)`, and Dust will try to compile `src` and register it under the original name. Sometimes, you might want to dynamically load one of several templates based on the original name. This change adds `cb(null, compiledTemplate)` so you can load any template from cache or the filesystem, register it manually (or not at all), and render it. The returned template will NOT be registered under the original name. Also: * Require dust.config.cache === false. This saves the people doing `dust.config = { whitespace: true }` * Call `onLoad` with `context.options` if it has 3-arity. Create options by calling `dust.makeBase(globals, options)` * Alias dust.makeBase => dust.context

Items already in the cache will be preserved but not used. New items will not be added to the cache.

Add a `cb(null, compiledTemplate)` signature to `dust.onLoad` Calling the `onLoad` callback with a compiled template function will use this template to satisfy the load request. The template is not automatically registered under any name when passed to the callback, so the `onLoad` function should handle registration as it needs. `dust.cache` behavior has been changed slightly. Before, setting it to false would blow away the entire cache on every render. Now, setting it to false just prevents new templates from being added and cached templates from being used, but if it's set to true again previously-cached templates will be ready to use.

sethkinast force-pushed the onLoad branch from 9f2e402 to 332b8d7 Compare April 20, 2015 23:53

sethkinast force-pushed the onLoad branch from 4145786 to a64a93f Compare April 23, 2015 20:04

sethkinast changed the title ~~[wip] Add a cb(null, compiledTemplate) signature to dust.onLoad~~ Add a cb(null, compiledTemplate) signature to dust.onLoad Apr 23, 2015

aredridel mentioned this pull request Apr 24, 2015

v0.1.8 does not support dustjs v2.7 krakenjs/adaro#61

Closed

jimmyhchan reviewed Apr 24, 2015
View reviewed changes

sethkinast force-pushed the onLoad branch from a64a93f to 8555c80 Compare April 24, 2015 19:01

Seth Kinast added 2 commits April 28, 2015 19:07

dust.config.cache only adjusts current cache behavior

34bc351

Items already in the cache will be preserved but not used. New items will not be added to the cache.

sethkinast force-pushed the onLoad branch from e3ab842 to 34bc351 Compare April 29, 2015 02:07

sethkinast merged commit f4f271d into linkedin:master Apr 30, 2015

sethkinast deleted the onLoad branch April 30, 2015 20:04

sethkinast mentioned this pull request Apr 30, 2015

dust.load -> internal breaks users of the old API #640

Closed

sethkinast mentioned this pull request Jul 23, 2015

Make use of context options to store @select state LinkedInAttic/dustjs-helpers#141

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a `cb(null, compiledTemplate)` signature to `dust.onLoad` #641

Add a `cb(null, compiledTemplate)` signature to `dust.onLoad` #641

sethkinast commented Apr 20, 2015

sethkinast commented Apr 21, 2015

aredridel commented Apr 21, 2015

sethkinast commented Apr 21, 2015

sethkinast commented Apr 21, 2015

sethkinast commented Apr 21, 2015

aredridel commented Apr 21, 2015

sethkinast commented Apr 21, 2015

sethkinast commented Apr 21, 2015

aredridel commented Apr 21, 2015

aredridel commented Apr 21, 2015

aredridel commented Apr 21, 2015

sethkinast commented Apr 21, 2015

aredridel commented Apr 21, 2015

aredridel commented Apr 21, 2015

sethkinast commented Apr 21, 2015

sethkinast commented Apr 22, 2015

aredridel commented Apr 22, 2015

sethkinast commented Apr 22, 2015

aredridel commented Apr 22, 2015

sethkinast commented Apr 22, 2015

aredridel commented Apr 22, 2015

aredridel commented Apr 22, 2015

sethkinast commented Apr 23, 2015

sethkinast commented Apr 23, 2015

aredridel commented Apr 23, 2015

sethkinast commented Apr 23, 2015

sethkinast commented Apr 23, 2015

aredridel commented Apr 23, 2015

aredridel commented Apr 23, 2015

sethkinast commented Apr 23, 2015

jimmyhchan Apr 24, 2015

sethkinast Apr 24, 2015

sethkinast commented Apr 28, 2015

sethkinast commented Apr 28, 2015

Add a cb(null, compiledTemplate) signature to dust.onLoad #641

Add a cb(null, compiledTemplate) signature to dust.onLoad #641

Conversation

sethkinast commented Apr 20, 2015

sethkinast commented Apr 21, 2015

aredridel commented Apr 21, 2015

sethkinast commented Apr 21, 2015

sethkinast commented Apr 21, 2015

sethkinast commented Apr 21, 2015

aredridel commented Apr 21, 2015

sethkinast commented Apr 21, 2015

sethkinast commented Apr 21, 2015

aredridel commented Apr 21, 2015

aredridel commented Apr 21, 2015

aredridel commented Apr 21, 2015

sethkinast commented Apr 21, 2015

aredridel commented Apr 21, 2015

aredridel commented Apr 21, 2015

sethkinast commented Apr 21, 2015

sethkinast commented Apr 22, 2015

aredridel commented Apr 22, 2015

sethkinast commented Apr 22, 2015

aredridel commented Apr 22, 2015

sethkinast commented Apr 22, 2015

aredridel commented Apr 22, 2015

aredridel commented Apr 22, 2015

sethkinast commented Apr 23, 2015

sethkinast commented Apr 23, 2015

aredridel commented Apr 23, 2015

sethkinast commented Apr 23, 2015

sethkinast commented Apr 23, 2015

aredridel commented Apr 23, 2015

aredridel commented Apr 23, 2015

sethkinast commented Apr 23, 2015

jimmyhchan Apr 24, 2015

Choose a reason for hiding this comment

sethkinast Apr 24, 2015

Choose a reason for hiding this comment

sethkinast commented Apr 28, 2015

sethkinast commented Apr 28, 2015

Add a `cb(null, compiledTemplate)` signature to `dust.onLoad` #641

Add a `cb(null, compiledTemplate)` signature to `dust.onLoad` #641