-
Notifications
You must be signed in to change notification settings - Fork 479
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.
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
Add a cb(null, compiledTemplate)
signature to dust.onLoad
#641
Conversation
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 |
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. |
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
}; |
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. |
So for dustjacket the simplified version seems like it should be something like:
|
I hate mixing the two string cases -- that's just going to lead to errors. I'd say letting the If |
That's legit. Will fix it. |
No context access is OK, right? You were just storing in there because you could? I dislike giving |
No, that's actually really needed. That's where i18n information lives. |
Why do you dislike it? |
(I was actually making a PR against this one to add context passing) |
Context is for templates and shouldn't be accessed by entities external to a template (à la functions that aren't context functions). |
Because? |
I can see the worry if it would extend the lifetime of context beyond the template, but |
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. |
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 This could mean adding some |
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 |
Context has a stack and globals currently. Maybe it needs some options that On Tue, Apr 21, 2015, 5:47 PM Aria Stewart notifications@github.com wrote:
|
That sounds delightful! |
The downside of that is that you then have to pass real contexts to The upside is that you're not polluting template context with options. |
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. |
One interesting hang-up is the specifics of |
|
😟? |
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! |
Nice, thank you. On Wed, Apr 22, 2015 at 7:41 PM, Aria Stewart notifications@github.com
Seth Kinast |
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? |
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. |
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. |
cb(null, compiledTemplate)
signature to dust.onLoad
cb(null, compiledTemplate)
signature to dust.onLoad
@jimmyhchan no rush, but I want your blessing before we talk about shipping this |
@@ -270,18 +308,19 @@ | |||
} | |||
}; | |||
|
|||
function Context(stack, global, blocks, templateName) { | |||
function Context(stack, global, options, blocks, templateName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: why this ordering? IMO global
and blocks
kinda go together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To pass the minimum number of params for makeBase. But this is completely
internal so it can be whatever if you like.
Ended up not being as bad as I thought, and I was able to gracefully use my new |
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.
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 compilesrc
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.