Skip to content
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 support for array as views #126

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

sundarj
Copy link

@sundarj sundarj commented Apr 20, 2015

Fixes #124.

I've added a fix based on your help, and now when views is an array, res.render checks both folders (I've tested). I've tried to match the existing codebase/style as closely as possible, but there's probably still a few things you can fix (like the _getView comment).

Hope it's good enough!

Adds a new private function _getView, that - if viewsPath is an array - compares it to viewPath stripped of the filename, and then does the usual _getTemplareName().
}

var self = this;
var stripFilename = /\/[^\/\\]+?$/;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work with Windows.

@ericf
Copy link
Owner

ericf commented Apr 20, 2015

I'm actually wondering if getting the view's "name" (the value passed to res.render()) is unknowable when the app is using multiple views dirs.

I'm not sure what should be considered the best match. We can use path.relative() on each of the views dirs to get back possible values to use as the view's "name", and that will work on Windows, but what should be the criteria to pick a winner?

var stripFilename = /\/[^\/\\]+?$/;

// Stripping filename (rather than indexOf) in case there are
// multiple views folders in the same parent directory
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is why we need a criteria for determine the best match. Should we go with, the file must exist in the dir, and have the shortest length to be the winner?

@sundarj
Copy link
Author

sundarj commented Apr 20, 2015

Ah, so if there happen to be multiple .handlebars files named the same thing in different views? Perhaps an error message asking one of them to be renamed, or giving each a unique name, e.g. res.render('index-0'), would work?

Something like this? path.normalize should take care of the backslash issue, but I'll see if I can test it tomorrow as well.
@ericf
Copy link
Owner

ericf commented Apr 20, 2015

If you have the following views dirs:

  1. /path/to/views/
  2. /path/to/views/pages/
  3. /path/to/other/views/

And the view that's being rendered is at: /path/to/views/pages/home.hbs

Using file.indexOf(dir) === 0 you'd get the following results:

  1. true
  2. true
  3. false

Using this._getTemplateName(dir, file) on the true results above, you'd get the possible view names:

  1. pages/home
  2. home

So the question is which one of these do you pick? Is it simply the shortest one? I think that in this case it would be reasonable to pick #2.

@sundarj
Copy link
Author

sundarj commented Apr 21, 2015

Yeah, that's why I used the strip-filename regex. With those three views dirs, you'd only get one possibility: /path/to/views/pages/. But I do agree it'd be reasonable to pick #2, and it could be overridden by specifiying pages/home, I suppose?

@ericf
Copy link
Owner

ericf commented Apr 22, 2015

and it could be overridden by specifiying pages/home, I suppose?

Nope it can't because no matter what's specified to res.render(), Express always passes the full path to the view file into the template engines.

@HenrikGr
Copy link

Is this fixed?
If so, which version do I need?

@samuelfine
Copy link

Taking a step back: much of this would be resolved if view.render() simply included the name of the view, right? View.prototype.render in /express/lib/view.js has access to this.name, but doesn't pass it along to this.engine(). If we knew the view name, we'd know—in the context of @ericf's example—that the view is, in fact, pages/home and not home.

@duncanhall
Copy link

Is there any update/decision on this? Restricting to a single views folder is really quite limiting.

@vanraizen
Copy link

Just ran into this issue today as well, would also like to know if there are any updates.

@hguillermo
Copy link

I really need this feature too. @ericf are you planning to merge this? are you waiting for something?

@hhope1271
Copy link

Also really need this feature.

I noticed after going through express-handlebars.js that nothing seems to need the view property. By removing that property and commenting out this line: view = this._getTemplateName(path.relative(viewsPath, viewPath));, then express-handlebars seems to work just fine with an array of view paths.

My question is: Is there any reason I shouldn't be pulling out the view property of the options object?

@gabrielerzinger
Copy link

Any updates on this? Did anyone cloned this repo and merged this...?

@UziTech
Copy link

UziTech commented Oct 23, 2020

Development has moved to https://github.com/express-handlebars/express-handlebars/ if you would like to create a PR on that repo we could add this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Array for views gives "TypeError: Arguments to path.resolve must be strings"
10 participants