ericf / express-handlebars

A Handlebars view engine for Express which doesn't suck.
BSD 3-Clause "New" or "Revised" License
2.31k stars 382 forks source link

Support for run-level partialsDir #118

Open nealshail opened 9 years ago

nealshail commented 9 years ago

I'm developing a site with a 'themeable' single route (or group of route). This is so a we can customise a given page easily using partials as hooks. For the majority of the other pages (the administration backend) no additional partials are required. So I've added the ability to pass an additional partialsDir while calling res.render()

res.render('viewer/index', {
        layout: false,
        partialsDir: req.theme.partialsDir,
    });

this overrides any default partials with the themes partials. I've also made it possible to send in array of partials..

res.render('viewer/index', {
        layout: false,
        partialsDir: [req.default.partialsDir,req.theme.partialsDir]
    });
ericf commented 9 years ago

What does the code look like that you have to use today without this feature?

[req.theme.partialsDir, req.default.partialsDir]

I think this should be reversed: defaults coming before the theme's partials.

nealshail commented 9 years ago

yes you are correct, it should be the other way around to override. I miswrote the pseudocode and have updated the comment now.

ericf commented 9 years ago

I think it might be confusing to have two partials related render-time options:

Which one takes precedence if both are provided?

I'm not sure how popular these two features would be, but we could change what you're proposing here to be expressible using the public API without adding a new render-time option:

res.render('viewer/index', {
  layout: false,
  partials: exphbs.getPartials({
    cache: req.app.get('view cache'),
    dirs: [
      req.default.partialsDir,
      req.theme.partialsDir
    ]
  })
});

To implement this, we could add a option.dirs to the getPartials() method that follows the same signature as partialsDirs config option, and it would default to this.partialsDir.

nealshail commented 9 years ago

yes this could work, though it adds an extra dependancy to exphbs which breaks the abstraction of the generic templating api that req.render() provides. At the moment the exphbs object is created during express configuration and startup, but my viewController code is way over the other side of my project, so i'd have to store it globally somewhere to be able to access it. It just feels like I'm adding a dependency to something 'under the hood'.

A natural partials Hierarchy does seem logical as you suggest.

ericf commented 9 years ago

I agree that breaking the abstraction is not great. Let me think about this some more… and let's see if we can come up with something that's easier for people to reason about and understand how the concepts of partials and partialsDir would interact.

nealshail commented 9 years ago

I was thinking about the partials parameter.. In it's current form it also has a dependancy on exphbs for it to be useful, am i right, it will have the same problem?

When i first saw the feature i thought it would uniquely work with files and intuitively take the form partials: { mypartial:"path/to/the/partial/file.hbs"} rather than the actual promise of the resolved template using the exphbs instance interface.

If one wanted to add a single partial in the form of a small generated string (like in the advanced example) wouldn't it most likely be better to pass it in as a regular handlebars {{parameter}}, and just construct all the elements on the fly before sending it in, at this stage one would be in the javascript code anyway... Forgive me but i have difficulty seeing the use case for the single partial parameter which isn't a file. but maybe I'm just biased..?

nealshail commented 9 years ago

for my use case I could also add the partials manually (as i know which partials are included in the specific view I'm loading).. if i could have an interface to pass in many partials at a time, i.e.

partials: [{ mypartial:"path/to/the/partial/file.hbs"}, {myotherpartial:"path/to/the/other/partial/file.hbs"}]

this may work for me in place of the partialsDir solution.

nealshail commented 8 years ago

Hi Ericf, Sorry it's been a while, this project has been on ice for a while.. Any more ideas on adding this feature for me?