leshill / handlebars_assets

Use handlebars.js templates with the Rails asset pipeline.
MIT License
647 stars 159 forks source link

Rails 4.1 fixes #112

Closed dgraham closed 9 years ago

dgraham commented 9 years ago

A couple fixes for the handlebars Sprockets engine in Rails 4.1. The engine registration issue was preventing the app from booting. The file extension issue causes HandlebarsTemplates to be undefined once the JS bundle reaches the browser.

AlexRiedler commented 9 years ago

@dgraham could you explain the file-extension change a little more (cause it would potentially be a breaking change to the API).

dgraham commented 9 years ago

It's possible the file extension without the period separator previously worked by accident. I tried using the separator here after looking through a couple other Rails engines:

AlexRiedler commented 9 years ago

@dgraham sorry it took so long to get back on this, focused on other stuff right now.

I booted a new rails application with 4.1.7; without this change and it works perfectly. So now I am curious what setup is causing this error to occur on your side? could you expand on this? what is not boot properly? app.assets is not defined?

dgraham commented 9 years ago

Right, app.assets is nil. It's curious that the sass engine checks for app.assets and the ember engine just ignores app entirely and goes straight to Sprockets class methods.

I don't know enough about booting the asset pipeline to know which way is correct. Maybe @josh can tell us what's preferred these days.

josh commented 9 years ago

.foo is preferred, but either should work in sprockets 2/3.

4.x will require .foo https://github.com/sstephenson/sprockets/commit/efe7d8190b9b525281fe6ac737d3ca2894ac56cd

AlexRiedler commented 9 years ago

@dgraham okay, looked into this a bit more; going straight for sprockets makes it global to all sprockets engines (which is most likely fine for the normal use case of this gem), checking for app.assets seems to be the right thing to do though; but sometimes its not there which means it might never be registered ... hmm tempted to just merge this.

dgraham commented 9 years ago

:zap: