Fix: Ensure assets without extensions are handled correctly #1115
Conversation
|
@doits @joshuay03 would you like to test this PR in your apps and confirm this solved the issue for you? |
@d4rky-pl I can confirm that this fixes the issue we were having. Thank you for working on this, and the original PR which seems like a super useful feature! |
|
@unixmonkey do you need anything else or can we release this as 2.8.1? :) |
There was a problem hiding this comment.
this is a cool fix, and I can throw a CI run with my app running with your branch. but if I only have assets with explicit extensions, it won't change a thing, if my code review is correct.
so I wonder, what if we want extensions to be explicit? I don't know.
|
@mathieujobin I have no idea either but requiring extensions would be an API breaking change and I wanted to avoid that (especially considering the library is more or less on life support at this point). My solution is based partially on how Sprockets solve that (in |
|
One other thing we could do is make it fail when there's more than one asset with the same name before the extension to avoid unexpected situations (like having |
|
Any plans to release |
| assets = Rails.application.assets_manifest.assets | ||
|
|
||
| if File.extname(path).empty? | ||
| assets.find { |asset, _v| File.basename(asset, File.extname(asset)) == path }&.last |
There was a problem hiding this comment.
Unfortunately this does not work for nested directories of assets.
for example company/logo/some_variant.svg
Here is the the fix:
assets.find do |asset, _v|
directory = File.dirname(asset)
filename_without_extension = File.basename(asset, File.extname(asset))
(directory == "." ? filename_without_extension : File.join(directory, filename_without_extension)) == path
end&.last
|
Added more tests and a fix, good catch @chaadow! Let me know if you can think of any other cases where this could break so I can add more tests |
|
@unixmonkey let me know if you need anything else to have this released. If there are any further bug reports related to this change, either before or after this PR is merged, feel free to @ me as well |
|
@unixmonkey Hi! is it possible to release this fix 🙏🏼 |
|
This is now published in version Thank you all for your help! |
This PR fixes the issues reported after #1094 and related to the asset manifest implementation not looking for the assets properly.
As a side note, this will most likely be my last contribution to this project. The fix itself took me 5 minutes to implement but adding those four simple test cases took... 1.5h. I wish I was joking 🥲
If anyone still has energy it would be great to at least figure out how to upgrade Mocha to the newer version as this one has a bug that prevents stubs from clearing properly (though it would be amazing to get rid of it completely and switch to rspec)