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

Added support for annotate_rendered_view_with_filenames #122

Merged
merged 1 commit into from
Mar 14, 2024

Conversation

iainbeeston
Copy link
Contributor

ActionView's erb template handler can annotate html with comments to indicate which file any given snippet of html comes from. This is very useful in development, but it isn't supported by better-html.

This adds equivalent support to better-html, using the same approach that ActionView takes, so it should work automatically for anyone switching from rails' default erb handler.

@iainbeeston
Copy link
Contributor Author

Fixes #76

Copy link
Member

@rafaelfranca rafaelfranca left a comment

Choose a reason for hiding this comment

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

Can we add a test for this new behavior?

lib/better_html/better_erb.rb Outdated Show resolved Hide resolved
@iainbeeston
Copy link
Contributor Author

@rafaelfranca Thanks for that feedback, I've refactored it to use internal config and I've also added a test for the new behavior. Please let me know if that's working the way you'd expect

@iainbeeston iainbeeston force-pushed the patch-3 branch 4 times, most recently from ff36a2e to e25615a Compare March 14, 2024 16:01
ActionView's erb template handler can annotate html with comments to indicate which file any given snippet of html comes from. This is very useful in development, but it isn't supported by better-html.

This adds equivalent support to better-html, using the same approach that ActionView takes, so it should work automatically for anyone switching from rails' default erb handler.
@rafaelfranca rafaelfranca merged commit b0fc796 into Shopify:main Mar 14, 2024
19 checks passed
@etiennebarrie
Copy link
Member

This doesn't work with Rails 6.0 because the config was only added in 6.1: rails/rails@a673ce6

The CI here passes because it doesn't run the initializers I guess? We could just bump the minimum required version for Rails since 6.0 is no longer supported anyway.

@iainbeeston
Copy link
Contributor Author

@etiennebarrie Does it throw an error? Or just not work on rails 6? It’s impossible to make this work on rails 6 because rails 6 overrides the preamble and postamble options for erubis

@etiennebarrie
Copy link
Member

Yes it prevents booting the app entirely, for example running bin/rails db:setup fails in Shopify/maintenance_tasks:

NoMethodError: undefined method `annotate_rendered_view_with_filenames' for ActionView::Base:Class
/home/runner/work/maintenance_tasks/maintenance_tasks/vendor/bundle/ruby/3.0.0/gems/better_html-2.1.0/lib/better_html/railtie.rb:11:in `block (2 levels) in <class:Railtie>'

@D-system
Copy link

Few hours ago, I opened an issue for this #129
I was not aware of this discussion.

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.

4 participants