RandomEtc / ejs-locals

Express 3.x layout, partial and block template functions for the EJS template engine.
298 stars 63 forks source link

Use settings.views to find the layout directory #8

Closed ball-hayden closed 12 years ago

ball-hayden commented 12 years ago

The attached could possibly be tidied up a little more, but uses options.settings.views as the layout directory rather than trying to find a layout in the same directory as the file to be rendered.
This allows file trees such as the following:

root
   |-layout.ejs
   |-index.ejs
   |-user
      |-edit.ejs
      |-list.ejs

where edit.ejs and list.ejs use the same layout as index.ejs. Previously, index.ejs would have correctly been rendered with layout.ejs (assuming app.locals({ _layoutFile: 'layout.ejs' }); had been set), but edit.ejs and list.ejs would have been looking for a layout.ejs file in the user directory.

This may also help with your FIXME on line 110, as I think view engine should be accessible using options.settings['view engine']. I haven't changed that in this pull request as I'm not quite sure what you want to do with it.

RandomEtc commented 12 years ago

Thanks for this. There are quite a few different issues that folks are trying to address and I'm having a hard time keeping them straight with the limited time I have to maintain this module.

My goal is to have the _layoutFile property behave exactly as layout did in Express 2.x, and for the layout() function in templates to behave as consistently as possible with this.

It will help me merge this pull request (and others) if you could add tests that fail without the change and that pass with it. I'll try to test and merge it myself regardless but it might take me a few days to resolve.

ball-hayden commented 12 years ago

No problem, and I appreciate that you may not have much time to maintain this. I have no experience writing tests for node, but will try and have a go over the next few days.

ball-hayden commented 12 years ago

Right... I've added a test which I think does the job. With the old code, this will fail. With my suggested code, this will succeed.
I've changed the devDependencies part of package.json to require express 3.0.0beta4 as I couldn't get any of the existing tests to run successfully without (I'm not quite sure why, but hey...)

RandomEtc commented 12 years ago

Awesome, thanks. I'll check this out soon. The way npm resolves the beta dependencies of Express probably explains the problem you ran into. The release is up to 3.0.0rc4 so hopefully goes "official 3.0" any day now.

RandomEtc commented 12 years ago

OK, I had a chance to look at this and I don't think it's the right thing.

If I have a subfolder in my views folder:

views
   |-layout.ejs
   |-index.ejs
   |-subfolder
      |-sublayout.ejs
      |-subindex.ejs

Then I want to be able to do this inside subindex.ejs:

<% layout('sublayout') %>

If I want subindex.ejs to use layout.ejs I would do:

<% layout('/layout') %>

Or just:

<% layout(true) %>

I don't have a test for these cases and I think they currently don't work as I'd planned. I'll figure that out first but I'll keep your use case in mind too! I think using the settings.views path is definitely on the right track, but I would like layouts to be relative to the calling template.

Stand by!

RandomEtc commented 12 years ago

OK, I just pushed 0.2.0 which should respect absolute layout paths (and the default is now /layout.ejs) - if the path is absolute it is resolved relative to options.settings.views. I also use options.settings['view engine'] where possible.

Please give this a try. If it doesn't fix your issue then please hassle me. I used your test for subfolders and added a new one so that there shouldn't be unintended side-effects.

Thanks for all your input and sorry for the delay!

ball-hayden commented 12 years ago

Many thanks - that looks good to me. I'll check it properly at some stage though. Thanks for your time (no worry about the delay ;-) ).

RandomEtc commented 12 years ago

Just published 0.2.5 which catches the issue with more recent express release candidates. The API for app.locals had changed which broke my tests.