Ensure helpers instance is consistent across renders #1085
Conversation
BlakeWilliams
force-pushed
the
bmw-ica/memoize-helpers
branch
from
7ec895c to
8afbfe8
Compare
BlakeWilliams
changed the title
Co-authored-by: Ian C. Anderson <[email protected]>
BlakeWilliams
force-pushed
the
bmw-ica/memoize-helpers
branch
from
f37f2fb to
858163b
Compare
|
|
||
| # This allows ivars to remain persisted when using the same helper via | ||
| # `helpers` across multiple components and partials. | ||
| @__vc_helpers ||= original_view_context || controller.view_context |
There was a problem hiding this comment.
We looked at upstreaming a change to rails/rails that would memoize controller.view_context but the intention seemed to be that a single instance should be instantiated and threaded throughout the render chain. I'm curious if anyone has more context or thoughts about attempting to upstream that to resolve the issue with the intention of eventually reverting this change.
| @@ -28,6 +28,8 @@ class Base < ActionView::Base | |||
| class_attribute :content_areas | |||
| self.content_areas = [] # class_attribute:default doesn't work until Rails 5.2 | |||
|
|
|||
| attr_accessor :original_view_context | |||
There was a problem hiding this comment.
Is this threadsafe? I wonder if CurrentAttributes would be a more Rails-y way to pull this off.
There was a problem hiding this comment.
I don't know if it is threadsafe as it is, but I don't think CurrentAttributes is the way to go here.
A word of caution: It's easy to overdo a global singleton like Current and tangle your model as a result. Current should only be used for a few, top-level globals, like account, user, and request details. The attributes stuck in Current should be used by more or less all actions on all requests. If you start sticking controller-specific attributes in there, you're going to create a mess. (source)
There was a problem hiding this comment.
Is this threadsafe? I wonder if CurrentAttributes would be a more Rails-y way to pull this off.
It's not, but the only time I think you would run into issues is if you were sharing a component instance across threads. CurrentAttributes could be an interesting way to tackle the problem though.
There was a problem hiding this comment.
Thinking about it a bit more, I don't think it's much different than the existing code since we set @view_context as a property on the component in render_in. Both aren't thread safe since any time we render the same instance in separate threads with a different view_context we risk using the incorrect view_context.
We're also memoizing things like helpers so component instances should probably never be shared between renders.
Co-authored-by: Joel Hawksley <[email protected]>
Currently helpers do not re-use the same view_context which can cause subtle bugs when using ivars in components.
This adds a failing test for this unexpected behavior.
Co-authored-by: Ian C. Anderson [email protected]