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

feat: Add vite_picture_tag for Rails 7.1 #409

Merged
merged 2 commits into from
Oct 5, 2023

Conversation

mattbrictson
Copy link
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

actioncable (6.1.3.1)
actionpack (= 6.1.3.1)
activesupport (= 6.1.3.1)
actioncable (7.1.0)
Copy link
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.

@@ -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')
Copy link
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
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
@ElMassimo ElMassimo merged commit 4e3762a into ElMassimo:main Oct 5, 2023
13 checks passed
@ElMassimo
Copy link
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
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
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
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
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