-
Notifications
You must be signed in to change notification settings - Fork 122
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
feat: Add vite_picture_tag for Rails 7.1 #409
feat: Add vite_picture_tag for Rails 7.1 #409
Conversation
62e7691
to
fa4b1ba
Compare
actioncable (6.1.3.1) | ||
actionpack (= 6.1.3.1) | ||
activesupport (= 6.1.3.1) | ||
actioncable (7.1.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗒️ I updated the lock file to be able to test against Rails 7.1.0. Let me know if I should split this into a separate commit or PR.
@@ -79,6 +79,17 @@ def vite_image_tag(name, **options) | |||
image_tag(vite_asset_path(name), options) | |||
end | |||
|
|||
if defined?(Rails) && Rails.gem_version >= Gem::Version.new('7.1.0') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I had to add the defined?
check to work around certain test failures. I assume that Rails
will be defined when this file is loaded in a real app. Is there a better way to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That approach would have worked, but changed my mind as I'd like to provide more information in case users see it in the docs and try to use it in an older version of Rails.
Rails 7.1 introduced a `picture_tag` helper.[^1] This commit adds a corresponding `vite_picture_tag` helper that handles resolving assets via Vite, but otherwise behaves the same as `picture_tag`. It is only exposed for Rails >= 7.1.0. [^1]: rails/rails#48100
fa4b1ba
to
a5827d4
Compare
… an older version of Rails
Nice addition, thanks Matt! Released in |
def test_vite_picture_tag | ||
assert_equal <<~HTML.gsub(/\n\s*/, ''), vite_picture_tag('images/logo.svg', 'images/logo.png', class: 'test', image: { alt: 'Logo' }) | ||
<picture class="test"> | ||
<source srcset="/vite-production/assets/logo.322aae0c.svg" type="image/svg+xml" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize this PR is ~9 months old, but this line has different output for me.
Running Rails 7.1.3.4, the type
attribute is different. I see a failure like:
Failure:
HelperTest#test_vite_picture_tag [/Users/mjankowski/repos/vite_ruby/test/helper_test.rb:186]
Minitest::Assertion: --- expected
+++ actual
@@ -1 +1 @@
-"<picture class=\"test\"><source srcset=\"/vite-production/assets/logo.322aae0c.svg\" type=\"image/svg+xml\" /><source srcset=\"/vite-production/assets/logo.f42fb7ea.png\" type=\"image/png\" /><img alt=\"Logo\" src=\"/vite-production/assets/logo.f42fb7ea.png\" /></picture>"
+"<picture class=\"test\"><source srcset=\"/vite-production/assets/logo.322aae0c.svg\" type=\"svg\" /><source srcset=\"/vite-production/assets/logo.f42fb7ea.png\" type=\"png\" /><img alt=\"Logo\" src=\"/vite-production/assets/logo.f42fb7ea.png\" /></picture>"
Maybe related to rails/rails#48266 ?
Curious to see a CI run after #467
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mjankowski I can't reproduce what you are seeing locally. Using vite_ruby @ main, and running bundle up
to get the latest gems, the test still passes for me:
$ bundle exec rake test TEST=test/helper_test.rb
...
14 tests, 36 assertions, 0 failures, 0 errors, 0 skips
$ bundle show rails
/Users/mbrictson/.rbenv/versions/3.1.1/lib/ruby/gems/3.1.0/gems/rails-7.1.3.4
Digging through the Rails source, my guess is that ActionView::Template::SimpleType
is somehow being used in your environment, causing the mime type lookup to return svg
instead of the correct image/svg+xml
.
I found this comment:
SimpleType is mostly just a stub implementation for when Action View is used without Action Dispatch.
So maybe Action Dispatch isn't being loading in your environment, for some reason?
https://github.com/rails/rails/blob/v7.1.3.4/actionview/lib/action_view/template/types.rb#L9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thanks ... think I tracked it down.
I have a branch going trying to get rid of some of deprecation noise during spec run, and I did a bit of branch jam-up on myself, and I think I had eager_load
disabled during that failure, which may explain the lack of the template being present (and it falling back to just ext string) during the run.
Description 📖
This commit adds a
vite_picture_tag
helper that handles resolving assets via Vite, but otherwise behaves the same as the Rails built-inpicture_tag
. It is only exposed for Rails >= 7.1.0. Fixes #386Background 📜
Rails 7.1 introduced a
picture_tag
helper.1The Fix 🔨
Added
vite_picture_tag
toViteRails::TagHelpers
with a corresponding test.Footnotes
https://github.com/rails/rails/pull/48100 ↩