leshill / handlebars_assets

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

Register template handlers very early on. Fixes #70. #73

Closed metaskills closed 7 years ago

metaskills commented 11 years ago

This does not have the app.assets guard. It would not work anyway since config.assets is not yet populated at this time nor. Besides, the only thing this was doing was not calling methods on Sprockets which is going to be there one way or another or fail much earlier on in your require's before engine.rb is required.

metaskills commented 11 years ago

I will also point out that it is likely possible that this require should be moved to the top.

metaskills commented 11 years ago

For example, this is my root require in less-rails.

https://github.com/metaskills/less-rails/blob/master/lib/less/rails.rb

metaskills commented 11 years ago

Some of them are not needed. For instance, tilt, sprockets requires that one anyway.

leshill commented 11 years ago

Hi @metaskills,

Can you update to the latest sprockets and try the original problem #70?

From looking at this, the original initialize code seems to be doing ‘the right thing’. expire_index! is called as part of the sprockets registration process. Given the lack of documentation for this, I would not be surprised if some gem(s) accidentally violate the expected protocol – but I am not yet convinced it is this gem.

For now I am rolling back the commit.

metaskills commented 11 years ago

Sure, but our 3.2.13 app was already on the latest version of sprockets that was allowed by our bundle. ActionPack requires ~> 2.2.1 and we have the latest 2.2 series, now 2.2.2 installed.

metaskills commented 11 years ago

Les, something else I noticed. This gem is implemented as an engine, however, all you need is a Railtie. Thoughts on that and possible benefits or not?

leshill commented 11 years ago

Hi @metaskills,

I am wondering if that is an issue with 2.2.x or not. I know you can reproduce the error, if you can get a failing example, I can help.

Good thought on the Railtie, will look at that as well.

metaskills commented 11 years ago

I am wondering if that is an issue with 2.2.x or not.

I grep'ed the v2.2.2 code and it had no expire_index! method. Based on this, my guess is that this is a 2.2 series bug and I it could be why the pattern of using before_initialize and the usage of class methods early on have evolved.

That said, is there anything else I can do or investigate to move this issue forward?

leshill commented 11 years ago

Hi @metaskills,

Odd, I am also looking at 2.2.2 and do see expire_index!?

Ideally you would have a simple Rails 3.2.13 app with a few sample assets that shows the problem using the same Gemfile.lock.

metaskills commented 11 years ago

Odd, I am also looking at 2.2.2 and do see expire_index!?

Whoa... not sure what I was looking at before.

Ideally you would have a simple Rails 3.2.13 app with a few sample assets that shows the problem using the same Gemfile.lock.

Agreed, however the gem that exposed this bug was an in-house rails engine that worked alongside devise to provide custom authentication.

I can get you a longer version of the orig stack trace from this ticket and reproduce to show some more debugging points. From what I am seeing the Processing module is included in the Base. Since its implementation for register_engine calls expire_index! it then hits the Index#expire_index! and raises the error.

From what I can tell, using the instance version of register_engine assumes that no one has called and pre cached the index, which seems risky. Perhaps other engines that may load before this one would do that for a valid reason. Maybe that is why the pattern of using the class method which does not ask the index to expire is a better alternative. Thoughts?

leshill commented 11 years ago

Hi @metaskills,

Any chance that you could create a shell gem that just calls register/whatever triggers the bug? The detailed stack trace is better than nothing, but a failing test case would be ideal.

metaskills commented 11 years ago

I can, but it may take a few days.

leshill commented 11 years ago

Great! Let me know when you have it.

metaskills commented 11 years ago

Less, I will not likely find time to a test this anytime soon. However, different teams at work outside of my own have also said that this PR fixed their issue as long as this did the require too. So I just added a commit that did that.

BTW, tests like this are hard. I have found that booting a railtie/engine for testing is going to always be a little different that in real world usage. When I did this for minitest-spec-rails' test_helper I got the best results by moving everything into the dummy app init to get me as close as possible. However, even then some boot issues always leaked thru.

Hopefully someone watching this ticket can contribute some time to this or I can round back to it in a few weeks.

AlexRiedler commented 10 years ago

I think I have fixed this in v0.17! @metaskills ; hopefully everything is working :S