Fix for cachePartials not honoring extension

[olsio]

closes TryGhost/Ghost#2459

cachePartials was loading partials as . which was overriding the
expected layouts with emacs backup file or other stale versions.

Fix is taking the file extension from the options object.

[ErisDS]

This fix seems like a sensible change to me, however it breaks the tests quite significantly (run grunt).

It seems there's no expectation that partials must be .hbs files, as many of the tests use .html.

I don't have enough understanding of how express-hbs is used outside of Ghost to know if this is relied upon, but as the tests break this expectation so extensively I'd hazard a guess that it is.

Perhaps a safe way to fix this is to require a dot followed by only [a-zA-z0-9]?

Interested to hear from @mgutz if he's around, what the expectations are known to be and if restricting to alphanumeric would be appropriate.

[ErisDS]

P.S. would be good to get travis setup in here!

[olsio]

Yeah breaking pretty much every test, looking into it. It's working in Ghost so I guess I just have to fix some test setup.

[olsio]

The existing tests were mixing .hbs files with .html for partials. For now I have aligned the tests to match .hbs everywhere. I am not sure if this mix is wanted if yes we could introduce another option like partial extension to allow mixing.

Another option to allow . would be to add some assert that would guarantee that no two files with the same prefix exist and throw an error if it does, so at least people know that we are resolving two possible views.

[ErisDS]

I am concerned about changing the underlying expectations in a backward-incompatible way here. The tests show that there is currently no expectation that partials must have the same extension as templates, and the documentation for the extension option states it is the extension for "templates", which can be understood to mean not partials.

Whilst I think your change makes sense, and that the extension should probably apply to partials as well as templates, this is not my project and as such I don't feel comfortable making breaking changes if it can be avoided.

What do you think about changing the fix to look for an alpha-numeric only extension e.g. *.[a-zA-z0-9]+? This should fix the problems we see in Ghost and the only case I can think that it might not work well with is multiple dot extensions e.g. html.hbs?

[olsio]

Indeed, this would most likely break some peoples themes that rely on the current behavior.

The alpha-numeric solution would fix the emacs ~ issue but loop.hbs loop.backup loop.old would still result in the same problem.

If we want to keep it backward compatible I would still vote for adding check for multiple partial files found that map to the same view name and then throw an Error. This at least would remove the frustration of failing silently and wasting peoples time.

[olsio]

Ok I guess this is not going anywhere. Will close the PR for now.

[ErisDS]

I don't think this needs to be closed - I just think it needs a bit more thought and discussion.

It results in very frustrating behaviour in Ghost, but fixing it could result in themes breaking. Therefore it's worth thinking about whether this change is worth the risk, and when or how we can safely make the change in Ghost.

There is, currently, ongoing work to improve the theme API inside of Ghost, and to start adding better deprecation messages - so that's worth keeping in mind too.

[ErisDS]

@mgutz how do you feel about this change?

To explain briefly, at the moment express-hbs doesn't care what the extension is for a partial. This makes some sense because registerPartial takes the name of the partial without the extension name, however this causes problems when developing:

If you want to quickly backup the state of a partial and then make some changes, you might save a copy of your file as mypartial.bk - but this version would be loaded first, and your changes to mypartial.hbs won't apply leaving you very confused.
Equally, if you use an editor like emacs that saves scratch as mypartial.hbs~ you'll find this gets loaded instead of your working version and again it's hard to figure out why.

This PR proposes that partials should also honour the extname option which is configurable already, by looking for .hbs files explicitly (or whatever is configured) we can resolve these issues.

I think it makes more sense that the extension be enforced for all files. I'm not sure why you'd want your partials to have a different extension, but they're frequently called .html in the tests, so I wanted to check.

This change is potentially breaking and so is the bump to handlebars 4, so I think it makes sense to do both of these and ship a 0.9?

[mgutz]

I see the advantages of being explicit with the extname. Because the new behavior and handlebars 4 breaks existing code it needs to be a major bump like 1.0.0.

[ErisDS]

Because the new behavior and handlebars 4 breaks existing code it needs to be a major bump like 1.0.0.

This rule does not apply to pre-1.0.0 according to semver, as long as we jump from 0.8.x to 0.9.0 we will have indicated the breaking change clearly enough. However, if you'd rather choose this moment to bump to 1.0 I'm more than happy to do that :)

[mgutz]

Our code would pull in 0.9 and break. I think it makes more sense to make it 1.0.0.

[ErisDS]

merged via #93