ericf / express-handlebars

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

`extname` documentation is misleading #188

Open bmcminn opened 7 years ago

bmcminn commented 7 years ago
OS:                 Windows 7 Pro 64 bit
Node version :      v6.0.0
express-handlebars: ^3.0.0

I find it very unintuitive to have to define extname in my express-handlebars instance to correctly resolve my layouts and partials filepaths.

app.engine('handlebars', hbs({
    extname:        '.hbs'
,   defaultLayout:  'views'
}));

app.set('view engine',  'handlebars');
-- /views
 |-- /layouts
 | `-- main.hbs       # this works
 |-- /partials
 | `-- ...
 |-- home.hbs         # this doesn't
 |-- home.handlebars  # this works
 `-- ...

Originally I was trying to use Using .hbs on my /views/FILENAME.hbs files resulting in a Error: Failed to lookup view "home" in views directory "/MY_PROJECT_PATH/views", but switching it back to .handlebars "fixed" the issue.

I would expect a view engine to resolve the file extension for all view files involved based on the standard Express engine behavior where you pass in the extension to be used on all view files.


After a fair amount of digging into this issue, I determined that I simply overlooked the documentation regarding how extname is utilized and was able to fix my problem per the examples on the docs:

app.engine('.hbs', hbs({
    extname:        '.hbs'
,   defaultLayout:  process.env.HBS_DEFAULT_LAYOUT
}));

app.set('view engine',  '.hbs');

I'm sure newcomers to Express have likely hit this disconnect before, but why is it that extname can't be derived from the engine instance extension value?


A related issue I found in the source for the engine is in the ExpressHandlebars.prototype._getTemplateName method: https://github.com/ericf/express-handlebars/blob/master/lib/express-handlebars.js#L322-L331

This section of the engine has a potential pitfall I think deserves discussing, and maybe some added testing around it.

The filePath argument contains the file extension we registered our view engine with, in my case .handlebars. However, the regex being used to strip the file extension is using the extname override, which I've determined is not influenced by the engine's registered value. So your regex fails to strip the extension and returns filename.handlebars because it doesn't match .hbs per my override.

I may be missing an implementation detail here, but I would assume if the goal to was return just the filename, having this kind of a mismatch left up to the developer implementing this with no intelligent debug messaging seems like a real problem to me.

Thoughts?

lvalencia commented 6 years ago

+1 I lost about half an hour of my time thinking that this applied to all handlebars files.

GeeWee commented 5 years ago

Just wanted to chime in that this also lost me about an hour of time until I found this issue.

bmcminn commented 5 years ago

UPDATE: since this is gaining some traction, I'd like to revisit the issue. Per the snippet I linked to back when, I think the best way to resolve this issue is to update the ExpressHandlebars.prototype._getTemplateName method to leverage the native path library and its basename() method and pulling the extname from the app view engine instance config.

I may take a stab at this later and issue a PR, along with updated Docs.

danielbayley80 commented 2 years ago

Is this still an issue? I just lost an hour of my life to this. Easy when you know how (thanks to this post) but it was far from obvious. It would have been quicker to debug if it spat out the full path it was looking for.

UziTech commented 2 years ago

express-handlebars has moved to a new repo https://github.com/express-handlebars/express-handlebars

If this is still an issue please create an issue or PR in that repo.