Usage of view_context leads to weird errors…
#124
Comments
|
I'm not sure how to re-use ERB's view-context. |
|
This also causes the |
|
So, after some investigation:
Therefore, we can't fix this without changing something in Rails, at least. |
|
@steveklabnik I was inspired to glance at this based on your Tweets, and I found this semi-horrible related approach to using content_for in controllers: https://gist.github.com/985457 I'm not sure if this type of approach is at all sane. Probably comes down to how much you trust the safety of the "Override this method in a module to change the default behavior" comment in the Rails source. ;-) |
|
Ha! Yeah. Hrm. Thanks though. |
|
I'm not using draper, but I'm using a homemade solution that calls |
|
hmm. it seems an initializer like that: AbstractController::Rendering.class_eval do
def view_context
@view_context ||= view_context_class.new(view_renderer, view_assigns, self)
end
endfix the issue too. perhaps rails should be caching the view context instance in the |
|
Yeah that'd be too hardcore of a monkey patch. I've been talking to @wycats about this a bit, but I really need to dig into what exactly Draper needs before we can determine what the real right thing to do is. |
|
@rafaelss Someone asked that a little bit ago, this was the answer: rails/rails#4906 |
|
@joevandyk that is a fantastic find, thank you! |
|
For what it's worth, the Cells project is running into this issue as well. |
|
If we could reset Draper's view_context at the time the first view is rendered (instead of in the controller), I think that would resolve some of the problems. |
|
@steveklabnik @rafaelss funny fact, it was actually me who asked the question both here and in the rails repo! 😃 |
|
I have the same problem: I get: I am using HAML in mailer's views: |
|
The rafaelss's fix doesn't work to me :-( |
|
Any update on this? |
|
Not really. It's going to require some sort of structural change in Rails to work 100% of the time, I think. |
|
Maybe now's the time to do it, before 4.0 comes out? I wonder if threads are related to the problems people are having with this? |
|
Yes, I've been having discussions with some rails-core people. It's not the threads. It's that we get a different copy of the view context than the one the actual renderer uses, so modifications of the context in the view renderer don't get copied over to the one that draper has a copy of. |
|
There's a simple workaround for this: class ApplicationController
skip_before_filter :set_current_view_context
def view_context
super.tap do |context|
Draper::ViewContext.current = context
end
end
endClearly Draper can't patch |
|
Right, the ultimate idea is that we just play nice and don't monkey-patch in general. That may not be an immediate option. It also might be better to do a monkeypatch like this now, and then get to a 'real' solution later. |
|
@rf- Really nice solution (for now). Never thought about the other way round! |
|
@steveklabnik The workaround that I pasted isn't monkey-patching, just inheritance. That's what I meant about providing a mixin—if users had to explicitly mix Draper support into their app's base controller, it would be easy to grab a reference to the final view context without patching anything. That would be kind of a regression in Draper's UI, though, so maybe monkey-patching is a better temporary solution. |
|
Is Draper::ViewContext.current a thread-local variable? |
|
Yes. The implementation is here: https://github.com/jcasimir/draper/blob/master/lib/draper/view_context.rb |
|
I was trying to reproduce this particular issue to attempt @rf-'s patch, but I couldn't actually reproduce it: https://gist.github.com/84e45c5ca17cd4a75f9c Are you doing something else, @halhappen? |
|
I'm kind of surprised that example doesn't break for you, but you should at least be able to reproduce it by moving the |
|
I was surprised too. Let me try that. |
|
Actually, I was just able to reproduce it using your example. What version of Rails are you using? |
|
Yep, that makes it bail. Awesome. |
|
Rails 3.2.2... apparently it's a little outdated. I have a draper_test rails app that I use to try to reproduce issues, so I generated it a bit ago. |
|
The biggest problem with the monkeypatch is that it breaks all the tests ;) That's our bad, though... I'm working on getting it done now. We were using |
|
@steveklabnik just bumped into this today, trying this: class TodoDecorator < Draper::Base
decorates :todo
def list_item(&block)
h.content_tag(:li, h.capture(&block), class: complete_state, id: h.dom_id(self))
end
private
def complete_state
if completed_at
'complete'
else
''
end
end
endthe alternative implementation using a helper def list_item(todo, &block)
complete_state = todo.completed_at? ? 'complete' : ''
content_tag(:li, capture(&block), class: complete_state, id: dom_id(todo))
endobviously works fine. Really interested in seeing if this can get fixed, since it'd be really awesome to do this sort of thing. |
We need to be able to let draper get a copy of the view_context no matter what happens. AC::Base doesn't really let us do that. So we have to monkeypatch. This was originally suggested in https://github.com/jcasimir/draper/issues/124#issuecomment-6954291 by @rf-. It appears to be the cleanest way to take care of this problem, and shouldn't affect any other things, since we keep the exact same semantics of view_context. Theoretically: Fixes #124 Fixes #109
|
Okay. Patch applied. Let's see how this goes. Please check if this works, everyone! |
|
Cool, thanks! A few small notes:
|
|
It actually doesn't work with
|
|
@steveklabnik I'd be happy to come up with a pull request that resolves the other issue and re-enables ActionMailer support, if you don't have anything in mind already. Should I? |
|
Feel free! I'm at OSCON, so I won't get the chance to do any work until tomorrow. I'll lay off and let you at it! |
|
OK, just opened #241 with a fix for ActionMailer. |
…, for example using
content_forboth in a Decorator class and directly in an ERB template won't work at all, cause Draper's instance ofview_contextisn't the same as ERB's. Is there any way to use the sameview_contextinstance as ERB?The text was updated successfully, but these errors were encountered: