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

Fix bad reference to Sprockets::Asset.pathname #106

Merged
merged 1 commit into from
Mar 5, 2020

Conversation

subdigital
Copy link
Contributor

I came across this error after upgrading a project to Sprockets 4.0 that uses inline_svg:

undefined method `pathname' for #Sprockets::Asset:0x00007f838e266b50

I'm not sure when this changed, but this PR adds in support for it. Let me know if there's anything more defensive we should do here... (like respond_to? checks for the old way?)

Also fixed up the credit URL in the comment, as that original repo has since been deleted.

Copy link
Owner

@jamesmartin jamesmartin left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @subdigital. ✨

I'm a little surprised that I haven't run into this before with my test app, which I use to test this gem on versions 3, 4, 5 and 6 of Rails. I guess Sprockets 4.0 was only released a month ago (even though it's been in beta for a couple of years), so I probably need to go update my test app! 😆

In any case, this looks good, but I'll give it some thorough testing on the older versions of Rails before merging. 👍

@olleolleolle
Copy link

The Sprockets change this is about happened between v3 and v4.

These two commits explain why the above change is good as it stands:

@kylefox
Copy link
Contributor

kylefox commented Feb 26, 2020

Just curious, is this backwards-compatible with Sprockets 3?

@jamesmartin
Copy link
Owner

jamesmartin commented Mar 5, 2020

Just curious, is this backwards-compatible with Sprockets 3?

I just tested this locally with a Rails 5 App using Sprockets 3 and it seems to work just fine. Was there anything in particular that you're concerned about, @kylefox? 🤔

Also, I merged @subdigital's changes into a temporary branch in this repo and let the integration tests run, which all passed.

@kylefox
Copy link
Contributor

kylefox commented Mar 5, 2020

Was there anything in particular that you're concerned about

@jamesmartin Nothing in particular, no — I just know our team has run into a handful of cryptic errors (originating in other libraries, not inline_svg) related to Sprockets 3, which unfortunately our app is stuck to for the time being 😕

Thanks for checking, I'll give this a whirl at some point myself. But don't hold up a merge/release because of me 👍

@jamesmartin jamesmartin merged commit d5d333e into jamesmartin:master Mar 5, 2020
@jamesmartin
Copy link
Owner

Released in patch version v1.7.1.

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