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

assets version doesn't depend on asset host if it's a Proc #140

Merged
merged 1 commit into from Apr 22, 2014

Conversation

pomnikita
Copy link
Contributor

this pr is replacement for #139

fixes #138

@@ -74,10 +74,16 @@ def configure(&block)
app.assets.version,
config.assets.version,
config.action_controller.relative_url_root,
config.action_controller.asset_host,
Copy link
Member

Choose a reason for hiding this comment

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

As we've already got a compact on this array, I think we can just use an inline conditional...

config.action_controller.asset_host unless config.action_controller.asset_host.respond_to?(:call),

wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can, but I found inline condition inside an array too complex, it doesn't even fit one line )

Copy link
Member

Choose a reason for hiding this comment

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

I prefer @matthewd version than doing the operation two times.

@scottjacobsen
Copy link

This fixes it for us. We are using a proc for the asset host. I'm really curious though - what is the use case where you would want to change the fingerprint on an asset just because the asset host changes? If the content of the asset hasn't changed what's the point?

@rafaelfranca
Copy link
Member

If asset host changes the content also change since the assets url inside
the stylesheet use the full url that contains the configured assets host
On Apr 21, 2014 11:57 AM, "Scott Jacobsen" notifications@github.com wrote:

This fixes it for us. We are using a proc for the asset host. I'm really
curious though - what is the use case where you would want to change the
fingerprint on an asset just because the asset host changes? If the content
of the asset hasn't changed what's the point?


Reply to this email directly or view it on GitHubhttps://github.com//pull/140#issuecomment-40952075
.

@scottjacobsen
Copy link

Okay. I would have expected the fingerprint to change because the content
of the stylesheet has changed. The portion of the fingerprint that is based
on the file content must be calculated before the expanded paths are
inserted into the stylesheet?

On Mon, Apr 21, 2014 at 11:00 AM, Rafael Mendonça França <
notifications@github.com> wrote:

If asset host changes the content also change since the assets url inside
the stylesheet use the full url that contains the configured assets host
On Apr 21, 2014 11:57 AM, "Scott Jacobsen" notifications@github.com
wrote:

This fixes it for us. We are using a proc for the asset host. I'm really
curious though - what is the use case where you would want to change the
fingerprint on an asset just because the asset host changes? If the
content
of the asset hasn't changed what's the point?

Reply to this email directly or view it on GitHub<
https://github.com/rails/sprockets-rails/pull/140#issuecomment-40952075>
.

Reply to this email directly or view it on GitHubhttps://github.com//pull/140#issuecomment-40952389
.

@matthewd
Copy link
Member

Well yes, that's the whole point: this is about the fingerprint of the source file(s), so sprockets knows whether it needs to recompile.

@robin850 robin850 changed the title assets version doesn't depend on asset host iff it's a Proc assets version doesn't depend on asset host if it's a Proc Apr 22, 2014
@pomnikita
Copy link
Contributor Author

rebased, inline conditional

rafaelfranca added a commit that referenced this pull request Apr 22, 2014
assets version doesn't depend on asset host if it's a Proc
@rafaelfranca rafaelfranca merged commit 9856b6d into rails:master Apr 22, 2014
rafaelfranca added a commit that referenced this pull request Apr 22, 2014
assets version doesn't depend on asset host if it's a Proc
@temochka
Copy link

Just spent quite some time figuring out why assets on two nodes got different fingerprints after a deployment. Thank you for fixing this! Hopefully the fix will be published to Rubygems soon.

rossta pushed a commit to challengepost/sprockets-rails that referenced this pull request Jul 24, 2014
assets version doesn't depend on asset host if it's a Proc
@masone
Copy link

masone commented Jul 29, 2014

Same here. Took me about half a day downtime to get to this. I hope we can expect a release soon :)

@Fjan
Copy link

Fjan commented Aug 1, 2014

Same here. I'm glad this is fixed but I don't care much for the approach. IMHO the digest of identical files should be identical and not depend on where they are hosted. The downside of the current approach is that the files on a staging server will always be different from a production server which would typically use a different asset host.

I understand that this is done as a hack to make sprockets realise the contents of a CSS file can change depending on the asset_host, but can't we think of something more elegant? Detecting whether the asset host is referenced anywhere in the file perhaps?

@bradrobertson
Copy link

@rafaelfranca Any chance this could be released in a 2.1.4 version? It's giving us inconsistent assets across multiple machines which is a pretty serious bug IMO.

@rafaelfranca
Copy link
Member

@braddunbar Released

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.

Environment assets version should not depend on asset_host
8 participants