ViewComponent / view_component

A framework for building reusable, testable & encapsulated view components in Ruby on Rails.
https://viewcomponent.org
MIT License
3.17k stars 404 forks source link

Update filter in source_location to use base_label #2009

Closed allan-pires closed 1 month ago

allan-pires commented 1 month ago

What are you trying to accomplish?

Fix ViewComponent compilation error that happens on a upstream ruby version.

Why do we need this change?

We need this to make sure templates are being correctly populated in a future ruby release.

The call stack filtering was added by https://github.com/ViewComponent/view_component/pull/281 to correct finding the template for child components. But unfortunately we started to see the following error happening in tests when running against the ruby version https://github.com/ruby/ruby/compare/d4a6c6521aa1a5208939a2cd981a13ca01a07d2a...1232975398a96af3070463292ec0c01e09a06c50:

ViewComponent::TemplateError: Couldn't find a template file or inline render method for ApplicationComponent.
    vendor/gems/3.4.0/ruby/3.4.0+0/gems/view_component-3.11.0/lib/view_component/compiler.rb:37:in 'ViewComponent::Compiler#compile'
    vendor/gems/3.4.0/ruby/3.4.0+0/gems/view_component-3.11.0/lib/view_component/base.rb:584:in 'ViewComponent::Base.compile'
    vendor/gems/3.4.0/ruby/3.4.0+0/gems/view_component-3.11.0/lib/view_component/compiler.rb:34:in 'ViewComponent::Compiler#compile'
    vendor/gems/3.4.0/ruby/3.4.0+0/gems/view_component-3.11.0/lib/view_component/base.rb:584:in 'ViewComponent::Base.compile'
    vendor/gems/3.4.0/ruby/3.4.0+0/gems/view_component-3.11.0/lib/view_component/base.rb:72:in 'ViewComponent::Base#render_in'
    vendor/gems/3.4.0/ruby/3.4.0+0/gems/actionview-7.2.0.alpha.94faed4dd4/lib/action_view/template/renderable.rb:16:in 'ActionView::Template::Renderable#render'

After some digging looks like this ruby change adds the method owner in backtraces, which in turn made the view component filtering stop working. The filtering would look for the label inherited in the call stack and filter them, but since that ruby change was introduced the label includes the method owner as prefix, so it’s being returned as ApplicationComponent.inherited:

Here's the caller_locations on the ruby sha 1232975398a96af3070463292ec0c01e09a06c50 (newer version):

"/<redacted>/app/components/application_component.rb:29:in 'ApplicationComponent.inherited'"
"/<redacted>/app/components/<redacted>.rb:5:in '<module:<redacted>>'"
"/<redacted>/app/components/<redacted>.rb:4:in '<top (required)>'"
"<internal:/<redacted>/vendor/ruby/1232975398a96af3070463292ec0c01e09a06c50/lib/ruby/3.4.0+0/rubygems/core_ext/kernel_require.rb>:37:in 'Kernel#require'"
...

And here's the caller_locations on the ruby sha 5124f9ac7513eb590c37717337c430cb93caa151 (our current version):

"/<redacted>/app/components/application_component.rb:29:in `inherited'"
"/<redacted>/app/components/<redacted>.rb:5:in `<module:<redacted>>'"
"/<redacted>/app/components/<redacted>.rb:4:in `<top (required)>'"
"<internal:/<redacted>/vendor/ruby/5124f9ac7513eb590c37717337c430cb93caa151/lib/ruby/3.3.0/rubygems/core_ext/kernel_require.rb>:37:in `require'"
...

This has caused the filter to stop working and templates to not be correctly populated for the child ViewComponent, which led to the error when trying to compile it.

What approach did you choose and why?

This goes back to work if instead of filtering by label we can use base_label, which doesn’t include the method owner prefix.

Anything you want to highlight for special attention from reviewers?

Thanks for your hard work 😄

joelhawksley commented 1 month ago

🤯 incredible! Thank you for your hard work! Mind adding a little more detail about why we needed this change in the changelog entry and in a code comment?

allan-pires commented 1 month ago

Mind adding a little more detail about why we needed this change in the changelog entry and in a code comment?

Of course! I also updated the PR description with more details, I hope it's not too much 🙊

reeganviljoen commented 1 month ago

@joelhawksley I think adding ruby-dev would be a very wise idea, maybe we should also consider testing YJIT compatibility aswell.

joelhawksley commented 1 month ago

@joelhawksley I think adding ruby-dev would be a very wise idea, maybe we should also consider testing YJIT compatibility aswell.

Sounds good. Would you be up for looking into adding one or both to CI?

reeganviljoen commented 1 month ago

@joelhawksley I would be happy to I will keep you posted 👍