Skip to content

feat: Add vite_picture_tag for Rails 7.1#409

Merged
ElMassimo merged 2 commits intoElMassimo:mainfrom
mattbrictson:features/vite_picture_tag
Oct 5, 2023
Merged

feat: Add vite_picture_tag for Rails 7.1#409
ElMassimo merged 2 commits intoElMassimo:mainfrom
mattbrictson:features/vite_picture_tag

Conversation

@mattbrictson
Copy link
Copy Markdown
Contributor

Description 📖

This commit adds a vite_picture_tag helper that handles resolving assets via Vite, but otherwise behaves the same as the Rails built-in picture_tag. It is only exposed for Rails >= 7.1.0. Fixes #386

Background 📜

Rails 7.1 introduced a picture_tag helper.1

The Fix 🔨

Added vite_picture_tag to ViteRails::TagHelpers with a corresponding test.

Footnotes

  1. https://github.com/rails/rails/pull/48100

@mattbrictson mattbrictson force-pushed the features/vite_picture_tag branch from 62e7691 to fa4b1ba Compare October 5, 2023 15:11
Comment thread Gemfile.lock
actioncable (6.1.3.1)
actionpack (= 6.1.3.1)
activesupport (= 6.1.3.1)
actioncable (7.1.0)
Copy link
Copy Markdown
Contributor Author

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.

image_tag(vite_asset_path(name), options)
end

if defined?(Rails) && Rails.gem_version >= Gem::Version.new('7.1.0')
Copy link
Copy Markdown
Contributor Author

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?

Copy link
Copy Markdown
Owner

@ElMassimo ElMassimo Oct 5, 2023

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
@mattbrictson mattbrictson force-pushed the features/vite_picture_tag branch from fa4b1ba to a5827d4 Compare October 5, 2023 15:15
@ElMassimo ElMassimo merged commit 4e3762a into ElMassimo:main Oct 5, 2023
@ElMassimo
Copy link
Copy Markdown
Owner

ElMassimo commented Oct 5, 2023

Nice addition, thanks Matt!

Released in vite_rails-3.0.17.

@mattbrictson mattbrictson deleted the features/vite_picture_tag branch October 5, 2023 16:53
Comment thread test/helper_test.rb
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" />
Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Contributor

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.

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.

Feature request: vite_picture_tag

3 participants