TryGhost / Ghost

Independent technology for modern publishing, memberships, subscriptions and newsletters.
https://ghost.org
MIT License
47.66k stars 10.4k forks source link

partials not cleared on theme switch #1203

Closed mbakaitis closed 10 years ago

mbakaitis commented 11 years ago

While working with a theme that has a custom pagination partial then swapping to one that does not, the new theme displays the prior theme's pagination partial.

For example, I set Ghost to use my custom theme that used a custom partials/pagination.hbs, restarted the server, then switched to casper. The casper theme pagination controls were rendering the code from my theme's custom partial.

This is on Ghost 0.3.3 on node 0.10.20 and Mac 10.8.5.

ErisDS commented 11 years ago

Thanks for raising this. @gotdibbs @jbillmann Any idea whether this is still a problem with the newer versions of express-hbs?

gotdibbs commented 11 years ago

@ErisDS My guess is yes. The way express-hbs currently re-caches partials is it just calls into handlebars to register the helper again. My guess is that duplicates entries not being overwritten, but rather just appended to the end of the array. Thus when we go to look for a partial with a specific name, it pulls the first one registered.

If that's the case, a fix for this would need to go in as a PR against express-hbs.

jbillmann commented 11 years ago

@ErisDS I verified that, as @gotdibbs predicted, this is still a problem with newer versions of express-hbs including if/when his latest PR is accepted.

ErisDS commented 11 years ago

Thanks, there are quite a lot of open issues in express-hbs to do with caching now: https://github.com/barc/express-hbs/issues?state=open seems it needs attacking. Anyone fancy it?

jbillmann commented 11 years ago

@ErisDS I can start looking into this express-hbs issue and others. I've spent enough time in it so I might as well go pro.

Btw, this one will likely involve a PR - I see @gotdibbs latest hasn't been accepted yet.

ErisDS commented 11 years ago

OK, I recommend getting a fix, making sure it's well tested & make the PR. We can try to make contact and get the PRs accepted, or otherwise we can setup a Ghost fork.

gotdibbs commented 11 years ago

@jbillmann Let me know if you have any questions while you're in there :)

jbillmann commented 11 years ago

Sounds good. :+1:

@gotdibbs Will do. I'll keep you posted on my progress, my up and down day schedule is my main concern.

gotvitch commented 11 years ago

The problem was the template for pagination was loaded during the starting of ghost and cached in memory. So once ghost launched, it can not change But there was some others problems in cache management of partial views. The PR to express-hbs doesn't resolve it because if the cache is enable you will execute the 'cachePartials' function only one time. So if first express-hbs is called for administration ghost site the partials views fof administration site will be cached, after if you go on the blog the cachePartials will not be called, so all partials views for the blog will not work. My solution is to manage cache of partial directly from ghost. When ghost start, partials views for administration site are cached. When theme changed all partials from the theme are cached. For templates helpers like pagination, if the template is found in the theme, it's cached else the default template is cached. What do you think?

gotdibbs commented 11 years ago

@Gotvitch Is there any reasonable way you think that we could update express-hbs instead to handle our use case? If possible I think we'd like to help express-hbs out since they've been helping us out as well.

gotvitch commented 11 years ago

In my opinion, there is no problem with express-hbs because in normal usage you create only one time the view engine. And the partial view are cached during the creation and it's good thing. I thing the main problem is to recreate the view engine at each request, it's not necessary.

Finally the only thinks we need when the theme changed are :

And I think it's very specific to ghost, the only think that we could improve in express-hbs is to expose methods for the management of partial view cache like 'cachePartialsFromDirectory, clearPartials'. We also need of method to update the global data send to view, because if we create the view engine only during the starting of ghost we cannot update the global data foreach request.

So in can propose a pull request to express-hbs to add these methods and if it's accepted, I could update my PR to ghost.

ErisDS commented 11 years ago

I completely disagree that this is Ghost-specific, just because we are the first people to start doing this kind of dynamic view loading with node certainly doesn't mean we will be the last.

We aren't the only people raising issues on express-hbs for partials, and doing this handling in express-hbs is very much the node way to go about it. Long term, we want to be moving as much of the Ghost code into modules as possible.

gotvitch commented 11 years ago

Ok, maybe I spoke too soon. We can handling this in express-hbs and other people could benefit. I have a idea, the problem with express-hbs is that we can not create isolated instances like in hbs. In our case we need two instances, one for administration site and a other for the blog that it's create when the theme change. The view engine is set in initViews but not re-create to keep the cache.

What do you think ?

ErisDS commented 11 years ago

@Gotvitch the changes you have made to express-hbs seem to be helping with our partial caching issues, even though we haven't updated to use multiple instances yet. I'm poking around at the moment to see what's going on and how we can use instances properly. Hoping that express-hbs will publish their latest changes soon :+1:

gotvitch commented 11 years ago

I also did some tests in Ghost with the theses changes and everything seems to work using 2 instance. I use the default instance of 'express-hbs' for the theme to access anywhere in Ghost using require. And I create an other instance for the administration site.

I also added to express-hbs the possibility to declare several directories for partial views, which is important for ghost. Because for templates helpers like pagination we have to verify the presence in the theme and if there is not, use the default template. So we just have to declare the partial directory of the theme and the default partial directory to handle this feature.

If you agree, when the new version of èxpress-hbs` will be published I could make a PR to fix all theses bug.

ErisDS commented 11 years ago

@Gotvitch PLEASE! :+1: