Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update filter in source_location to use base_label #2009

Merged

Conversation

allan-pires
Copy link
Contributor

@allan-pires allan-pires commented Apr 3, 2024

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 #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 ruby/ruby@d4a6c65...1232975:

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 😄

Co-authored-by: jasonkim <jasonkim@github.com>
@allan-pires allan-pires force-pushed the update-source-location-filter branch from 5b9b745 to 3388241 Compare April 3, 2024 22:25
@joelhawksley
Copy link
Member

🤯 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 allan-pires changed the title [WIP] Update filter in source_location to use base_label Update filter in source_location to use base_label Apr 4, 2024
@allan-pires
Copy link
Contributor Author

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 🙊

@allan-pires allan-pires marked this pull request as ready for review April 4, 2024 00:19
docs/CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@joelhawksley joelhawksley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Makes me wonder if we should test against HEAD for Ruby and not just Rails 🤔

Might y'all want to add yourselves to the contributors list on index.md?

@reeganviljoen
Copy link
Collaborator

reeganviljoen commented Apr 4, 2024

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

@joelhawksley
Copy link
Member

@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
Copy link
Collaborator

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

@joelhawksley joelhawksley merged commit f6dc5be into ViewComponent:main Apr 4, 2024
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants