Allow each partials dir to be configured

[joanniclaborde]

Since each partials dir can be specified as an object, each dir can be configured differently. This is useful for example to always cache distributed partials, that don't need to be reloaded at each request in development:

var hbs_config = {
  extname: '.hbs',
  partialsDir: [
    '/.../views',
    {dir: '/.../node_modules/third-party/views', namespace: 'third-party', cache: true}
  ],
  handlebars: Handlebars
};
var handlebars = express_handlebars.create( hbs_config );

[ericf]

that don't need to be reloaded at each request in development:

Are you actually finding this to be an issue during dev? If so, how many partials are being reloaded during dev and about how long is it taking to render the page?

[joanniclaborde]

Yeah, we need to add node_modules as a partials dir, so the users of our web server can automatically use partials from npm modules they install ({{> some-module/partial}}, where some-module is located in /.../site/node_modules/some-module). But since it doesn't take long for node_modules to be huge, scanning it to find the available partials takes 5-6 seconds on my machine (in dev), with just a few modules.

I tried to add some kind of list of folders to exclude when scanning for partials, but node-glob doesn't support exclusions, and the alternatives like node-walk still take about 2s to scan everything. And the resulting changes are much more complex than the solution above.

[ericf]

Okay, so you're figuring that you can always cache the walk through node_modules since in order to change what's in there, you're going to run npm <command> and it's safe to assume shutting down the server?

[joanniclaborde]

In my case yes, that's the best solution I could find. The first dev request is slow (5s) but then it's very fast.

[joanniclaborde]

I could also only only pick the properties I'm interested in for partials (cache and namespace), instead of merging the whole thing and creating potential problems. Do you have a preference?

[joanniclaborde]

Merging the options in the reverse order (dirOptions = utils.extend({}, dir, options);) doesn't work, since cache is always set in options, so the partials dir cache is always overwritten.

What about something like this? Only the cache option is merged in the render options:

...
    partialsDirs = partialsDirs.map(function (dir) {
        var dirOptions = {},
            namespace;

        // Support `partialsDir` collection with object entries that contain a
        // namespace.
        if (dir && typeof dir !== 'string') {
            dirOptions = {cache: dir.cache};
            namespace  = dir.namespace;
            dir        = dir.dir;
        }

        return this.getTemplates(dir, utils.extend({}, options, dirOptions)).then(function (templates) {
            return {
                templates: templates,
                namespace: namespace
            };
        });
    }, this);
...

[ericf]

I want passed-in options to trump the dir options, otherwise the caller can never control what's going to happen and get what they want.

Why would the cache prop always be set? If it has the value undefined we can consider that unset.

[joanniclaborde]

This is what I found through testing. Looking at the source code, Express seems to always set cache to true or false, in both the version we're using, 3.1.2, and the latest 4.8.8.

But I see your point, I don't know if my solution is possible in this case...

[ericf]

Okay, that will be an issue for how I was thinking we could handle this.

I feel like we should consider this a more advanced feature, and therefore be fine the solution ends up requiring the user to write code (instead of just configuration). How would you feel if the solution looked something like this in your app code?

var exphbs = require('express-handlebars');

// First create an instance with your standard config, including the _normal_
// partials dirs, but leave off the node_modules/ special case.
var hbs = exphbs.create({
    ...

    partialsDir: [
        {
            dir      :'./views/partials/',
            namespace: 'app'
        }
    ]
});

// Add to `partialsDir` by pushing on a new "dir" entry to the array that 
// contains the templates or a promise for the templates. This "dir" config 
// entry would also allow you to add a `namespace`, if desired.
hbs.partialsDir.push({
    templates: hbs.getTemplates('./node_modules/')
});

Then inside of getPartials(), when it iterates over partialsDir (which seems poorly named now, maybe it should change to simply partials?), if it encounters a partials dir config object with a templates property, it will simply use that value or promise for a value.

I feel that this yields more control and opens up the door for people like yourself to do more advanced things without needing to muddy up options objects and mixing them, etc. If you want to do something advanced for some set of partials, simply return a promise for the templates.

What do you think? Is this better, or too complex?

[joanniclaborde]

Good idea! I tried that and it seems to work very well. I'll review my changes then push another commit for review. Quick question: when I call hbs.getTemplates('./node_modules/') from my code, should I pass some options, or will the defaults be ok in my case?

[joanniclaborde]

I committed the changes here.

[ericf]

Quick question: when I call hbs.getTemplates('./node_modules/') from my code, should I pass some options, or will the defaults be ok in my case?

Yeah, because in this case your handing over a promise for a collection of templates. Once resolved, this promise will always be returning the same value, effectively caching the result.

[ericf]

I merged this in and made a couple of tweaks for dealing with the different possible partialsDir values, and making sure templates is always a promise.

[ericf]

It would be great if you could try out what's in master in your app to make sure things all seem to be working correctly. Let me know, and then I'll release v1.1.0.

[joanniclaborde]

Just played with master, everything is still working fine, and all my tests still pass. Thanks!!

[ericf]

:thumbsup: I'll shoot for releasing v1.1.0 this weekend.

[ericf]

@joanniclaborde just published v1.1.0 to npm.

[joanniclaborde]

Thanks a lot!!