3.4.1: Relative assets urls are converted into absolute ones. Breaking existing code.

[tvdeyen]

In 05dbab2 a breaking change was introduced and released as 3.4.1 as a hotfix for the already breaking 3.4.0

Before sprockets 3.3.0 a url like url(img/thing.gif) inside a folder loads the image from the current folder (the folder the file lives in) relatively. This is valid HTML/CSS and every css processor reads this correctly.

Now the url is converted into an absolute URL. This breaks a lot of existing frontend code. In a patch level release by the way.

Can we revert this please and release a new version? Then rethink changes made in 3.3.0 and 3.4.0 and release them as a 4.0 instead if a breaking change is intentional/necessary?

[tvdeyen]

You can see the breakage in this failing CI https://github.com/AlchemyCMS/alchemy_cms/runs/4300052710?check_suite_focus=true#step:12:127

and what was necessary to fix it

https://github.com/AlchemyCMS/alchemy_cms/pull/2218/files

as you can also see we have to use a relative url starting from the top most folder (the sprockets lookup path) but it is then turned into an absolute url.

Both behaviors seem wrong from what a browser (and a file system) would usually do.

Expected behavior:

url("img/some.gif")

from a file in a folder app/assets/stylesheets/components/style.css

should return a url

url("/assets/components/img/some.gif")

and not

url("/assets/img/some.gif")

while a

url("/img/some.gif")

should return

url("/assets/img/some.gif")

[rafaelfranca]

@jcoyne can you take a look on this one as well please? We should making the absolute path relative to the current asset, and that should fix the issue.

[jcoyne]

@rafaelfranca yes, I'll take a look today.

[jcoyne]

@tvdeyen is this still an issue after #485?

[nickcoyne]

I see this issue in versions 3.3.0, 3.4.0 and 3.4.1. No problems on 3.2.2.

[jcoyne]

@nickcoyne the code from #485 hasn't yet been released, but it is in the main branch.

[nickcoyne]

ok, thanks @jcoyne, I'll give that a try then.

[valscion]

This issue still affects us even with the changes in #485. Previously sprockets-rails output the filenames as relative and now the change to paths being absolute breaks our build.

I don't really understand the reasoning behind this change and why these tests are the expected ones:

sprockets-rails/test/test_asset_url_processor.rb

Lines 52 to 63 in 4e0f168

	def test_relative
	input = { environment: @env, data: 'background: url(./logo.png);', filename: 'url2.css', metadata: {} }
	output = Sprockets::Rails::AssetUrlProcessor.call(input)
	assert_equal("background: url(/logo-#{@logo_digest}.png);", output[:data])
	end
	def test_subdirectory
	input = { environment: @env, data: "background: url('jquery/jquery.js');", filename: 'url2.css', metadata: {} }
	output = Sprockets::Rails::AssetUrlProcessor.call(input)
	jquery_digest = 'c6910e1db4a5ed4905be728ab786471e81565f4a9d544734b199f3790de9f9a3'
	assert_equal("background: url(/jquery/jquery-#{jquery_digest}.js);", output[:data])
	end

Is there any way to make sprockets-rails still output the paths as relative like it used to do before v3.3.0? Our asset pipeline relied on sprockets-rails outputting the relative paths and the change in v3.3.0 seems to have made that impossible.

[jcoyne]

@valscion This is all driven by the new Rails 7 asset pipeline, which primarily uses sprockets-rails as a post-processor for cssbundling-rails. It was the fix for rails/cssbundling-rails#22

Your knowledge of sprockets-rails and it's traditional uses likely exceeds my own, so your help with this issue would be welcome.

It seems that AlchemyCMS may be an outlier in terms of how sprocket-rails is used since you have images in a subdirectory of app/assets/stylesheets. I'm still trying to wrap my head around this usage.

[jcoyne]

@rafaelfranca I'm way out of my depth here. I've looked but I'm not sure how to go about this.

[valscion]

Ok well good to know that this is driven by Rails 7 needs 👍. We'll pin our version to 3.2.2 as this backwards-incompatible change was released in v3.3.0 ☺️

[tvdeyen]

We'll pin our version to 3.2.2 as this backwards-incompatible change was released in v3.3.0 ☺️

As I asked above, can we bump the version to 4.0.0.pre with the changes for Rails 7 and revert the breaking changes from 3.3, 3.4 and release them as 3.3.1 and 3.4.2?

Then we can pin our projects to sprockets-rails ["~> 3.3", "< 4.0"] and Rails 7 can start using 4.0.0pre

@rafaelfranca @dhh are you fine with that?

[jcoyne]

@tvdeyen We use cssbundling-rails + sprockets-rails on a rails 6.1 app, so that might cause a breaking change for us. It could work if the dependencies didn't exclude rails 6.1. However, I don't think that this breakage was supposed to happen in rails 7. This is a bug that needs to be resolved for all users of this gem.

[tvdeyen]

@jcoyne could you use proposed 4.0.0.pre in your Rails 6.1 as well? I am not saying that sprockets-rails 4.0 should only work with Rails 7, but that Rails 7 uses sprockets-rails 4 as default if the breaking changes are necessary for Rails 7.

[jcoyne]

@tvdeyen So that everyone better understands the AlchemyCMS use case, are the tinyMCE image assets linked to without the hash digest? It might be that using cssbundling-rails wants all image assets to be digested and your use case is expecting non-digested assets.

[tvdeyen]

@jcoyne yes, but this is totally unrelated to this issue. The digest gets removed (aka a symlink created) via the non-digest-stupid-assets gem. That is not the issue we are seeing. The issue is as described above with relative urls and stylesheets and their images in the same folder (a common practice in JS themes). We fixed the tinymce icon font issues by adjusting our tinymce skin to use absolute paths instead. That was an easy fix as we own the code for the skin.

But another issue we are seeing is with the fontawesome icon font (and the css bundle they provide). The paths in the stylesheet (you are downloading from their website) are all relative. With sprockets-rails < 3.3 everything works as expected. With 3.3 and above it broke because sprockets-rails converts relative paths into absolute ones.

[jcoyne]

it broke because sprockets-rails converts relative paths into absolute ones.

It does convert them to absolute paths because it is trying to make them into digested assets. I don't think you want these to be digested assets and I don't think we have a way of deciding which assets should be digested and which shouldn't.

[tvdeyen]

It does convert them to absolute paths because it is trying to make them into digested assets

sprockets-rails 3.3.0 was able to provide digested urls without making them absolute.

[jcoyne]

@tvdeyen it didn't provide digested urls for every url() in the css, only those that used image-url or similar I believe. With cssbundling-rails, that would have to change as it emits regular CSS.

Right now, I don't think (my knowledge is limited), there is a way to keep the functionality you depend on and have the behavior that cssbundling-rails needs without some kind of feature flag.

[valscion]

Yeah for us, we were relying on sprockets-rails to not mangle url() at all and let it be as it was. We are generating our CSS using a different pipeline and pass the end result through Sprockets pipeline so that we get gzipping and can use the asset_sync gem. The separate pipeline already adds a cache bust hash to the generated CSS filename, so we also are using non-stupid-digest-assets gem to make our application work.

The change in sprockets-rails v3.3.0 suddenly changed the behavior of how url() is handled, and that to me, too, looks like a breaking change that should've been released in a major version release and not a minor one.

And I guess that's what this issue is about — discussing semantics of version numbering. Not about whether there's some sort of a "fix" for how to keep the early behavior.

There might also be other gems that have relied, maybe on purpose, maybe by accident, on the old behavior where url() behavior was not changed and only image-url was.

It's of course up to the maintainers of sprockets-rails to decide how they want to version the gem ☺️.

---

And yes, the way we use sprockets and sprockets-rails can seem to be quite unorthodox. We know that our way of using sprockets and sprockets-rails can be prone to breakage when a change to the Rails builtin asset pipeline is changed. And that seems to be the case for us here in sprockets-rails gem.

For me, I'm a bit sad that we have to pin the gem to v3.2.2 but I also understand that it's a price we must pay for fiddling with the asset pipeline as much as we do.

[valscion]

Seems like AlchemyCMS also uses the non-stupid-digest-assets gem and thus already ends up monkeypatching the way Sprockets works: https://github.com/AlchemyCMS/alchemy_cms/blob/047119d1ac8bb039452ed1fa60e15b3a387f8cec/alchemy_cms.gemspec#L48 and uses the gem with the tinymce part referenced in this issue: https://github.com/AlchemyCMS/alchemy_cms/blob/047119d1ac8bb039452ed1fa60e15b3a387f8cec/lib/alchemy/engine.rb#L18-L20

So the way I see it is that v3.3.0 breaks compatibility with the non-stupid-digest-assets gem. And that non-stupid-digest-assets has always been prone to breakage given that it does it's work by monkeypatching another gem.

Now that sprockets-rails has more features by also knowing how to handle url() natively without using image-url, the way the asset pipeline that required non-stupid-digest-assets to work must change. I don't know how that change will look like, though.

To me, it feels more like a burden for us end users who used non-stupid-digest-assets gem to figure this out. We were relying on a monkeypatch to keep our application working like it used to. And now here we are.

[jcoyne]

@valscion @tvdeyen thank you for you feedback on this problem. It's clear that the committers will have to make a decision about how to move forward. Rewinding a released feature is complicated, but we also shouldn't have broken existing apps. One thing I've learned is that sprockets-rails has very little test coverage, especially integration type tests. If someone is able to add those tests, I think this project could be a lot more stable.

[dhh]

In retrospect, yes, we probably should have bumped the major version. But that's more of a stylistic change. The end result would have been the same: some use patterns would have been incompatible. I don't see the trade off of reverting changes, pushing out a new 3.x release, then doing a separate 4.0 as being worth it at this point, though.

We can fix the relative-to-absolute conversion, though. So that the absolute path is translated relative to the path of the asset. @jcoyne, if you want to tackle that, please go ahead, and we can release that in 3.4.x.

[valscion]

So that the absolute path is translated relative to the path of the asset.

That might work for us, yes.

For example, we have the built CSS file living in production in here:

https://assets.venuu.se/assets/v2/bundle/fi/styles.433344a57edc53a908ed.css

And with sprockets-rails v3.2.2, that CSS contains this declaration for a SVG image:

.cFMs319K8uCdE_4aIE3wU::before {
  content: url(static/icon-sitting-white.72e233bb0680f4a70b717dcea570e673.svg);
}

Now this ends up downloading the SVG from here:

https://assets.venuu.se/assets/v2/bundle/fi/static/icon-sitting-white.72e233bb0680f4a70b717dcea570e673.svg

---

With sprockets-rails v3.4.1 that we tested, the CSS end result was something like this:

.cFMs319K8uCdE_4aIE3wU::before {
  content: url(/static/icon-sitting-white.72e233bb0680f4a70b717dcea570e673.svg);
}

So the place it tried to load the SVG from ended up being this one:

https://assets.venuu.se/static/icon-sitting-white.72e233bb0680f4a70b717dcea570e673.svg

...which was happily caught by our CI before it reached production.

For us, it would've been OK if the CSS ended up looking like this instead:

.cFMs319K8uCdE_4aIE3wU::before {
  content: url(/assets/v2/bundle/fi/static/icon-sitting-white.72e233bb0680f4a70b717dcea570e673.svg);
}

---

We've configured our application with:

Rails.application.configure do
  config.assets.version = 'v2'
  config.assets.prefix = File.join('/assets', config.assets.version)
  config.assets.digest = true
  config.assets.precompile << ['bundle/*']
end

and the CSS-file-to-be-precompiled can be found in path

app/assets/javascripts/bundle/fi/styles.433344a57edc53a908ed.css

while the SVG file referenced in CSS is in path:

app/assets/javascripts/bundle/fi/static/icon-sitting-white.72e233bb0680f4a70b717dcea570e673.svg

So during precompilation, all the necessary information should be there to go from

.cFMs319K8uCdE_4aIE3wU::before {
  content: url(static/icon-sitting-white.72e233bb0680f4a70b717dcea570e673.svg);
}

to this one:

.cFMs319K8uCdE_4aIE3wU::before {
  content: url(/assets/v2/bundle/fi/static/icon-sitting-white.72e233bb0680f4a70b717dcea570e673.svg);
}

We'd be OK even if the end result would get the digest added to it and become something like this instead:

.cFMs319K8uCdE_4aIE3wU::before {
  content: url(/assets/v2/bundle/fi/static/icon-sitting-white.72e233bb0680f4a70b717dcea570e673-331b0896fbbe3be736a015ecbb0382c7a837e7aef841467ceb5ab24bb0914d9c.svg);
}

[jrochkind]

It does convert them to absolute paths because it is trying to make them into digested assets. I don't think you want these to be digested assets and I don't think we have a way of deciding which assets should be digested and which shouldn't.

So, the need for digested asset references in some cases of course existed before. Is it that Rails 7 with css-bundler increases the cases where you want/need this?

As I understand it/have experienced it, previously, if you wanted digested assets referenced in a CSS file, you'd have to use something like the sass-rails helper asset-url(), or the ERB helper asset_url(). Which means you'd have to have your CSS going through SASS or ERB or something like that, yeah.

Sprockets on it's own didn't parse url() references and manipulate them. So you could also have url() references to local paths that were not digested. That's what you'd get ordinarily, when you needed digested references you'd use one of those helpers.

The intent of the change seemed to be to make sprockets parse CSS for all url() references, and turn them into digested versions if possible?

I'm not sure there's any way for that to happen backwards compatibly. In the sense that if some people in some cases had non-digested local path references to be in their CSS, which went through sprockets, and they were relying on this... and it's now impossible?

I guess it's not possible to meet the desired new use case, by having something in the CSS source indicate that a local digested path should be looked up, instead the need is that all url() references are to digested versions, that it be impossible to send something through sprockets with a relative path url() that is not a fingerprinted-path? I may not be explaining this right, because I do not follow the details of why this is a new requirement... but it does seem impossible to achieve it with backwards compat.

The benefit of releasing a backwards incompat change only in a new version, of course, is that people who have their Gemfiles locked to major version 2 won't get breakages when they update automatically, and people won't have to add the somewhat opaque requirement of locking to 3.2.2 to avoid the backwards incompat until they are ready to upgrade, instead they'll be able to lock to the conventional ~> 3.2.

[jrochkind]

Could a sprockets configuration option be added, which defaults to old behavior for backwards compat, but allows opt-in to new behavior to parse/transform all relative url() references, that I guess (but don't personally understand) is necessary for use with cssbundling-rails?

[rmacklin]

@jrochkind I've opened a draft PR to make this behavior configurable: #489. Can you either provide an example application that reproduces the problem or simply try that PR in your application? The expected behavior would be that when using that branch, your assets work (CSS url() references would not be rewritten), and if you then set Rails.application.config.assets.rewrite_css_urls to true it would enable the behavior that rewrites CSS url() references.

[valscion]

The approach in #489 would work for us and fix the issue with us not being able to upgrade ☺️

[rmacklin]

@valscion Just want to clarify - were you able to test PR #489 in an application that had previously experienced broken assets when upgrading sprockets-rails?

Despite opening that PR, I actually don't work on any applications that were affected by the issue, which is why I was asking above if @jrochkind or someone else could either provide an example application or test the PR in their own private application. I was waiting to take the PR out of the draft state until someone who was affected by this issue could verify the fix.

[valscion]

were you able to test PR #489 in an application that had previously experienced broken assets when upgrading sprockets-rails?

Yes, I took that PR to a test-drive. That run passed our tests so I'm fairly confident that we'd be able to ship a version of sprockets-rails with the changes in #489.

[dhh]

Option to turn off rewriting has been shipped in https://github.com/rails/sprockets-rails/releases/tag/v3.4.2.