Environment assets version should not depend on asset_host #138

Closed
pomnikita opened this Issue Apr 17, 2014 · 17 comments

Comments

Projects
None yet
10 participants
@pomnikita
Contributor

pomnikita commented Apr 17, 2014

Now assets.version depends on config.action_controller.asset_host, but it may cause
issues in case asset_host is a Proc like

  config.action_controller.asset_host = ->(path, request) {
    'http://some-cdn-depends-on-path-and-request.com'
  }

so on each initialization and precompile asset environment has different version

test-v2-some-path-#<Proc:0x007f8246822600@/path_to_config.rb-#{Sprockets::Rails::VERSION}

and therefore different digest and assets are not cached

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Apr 17, 2014

Member

Well as you note, it was recently added (#110, #69), so just undoing it doesn't seem likely to be a solution.

Perhaps we should just omit it iff it's a Proc? That obviously does leave the possibility that the proc's body has changed... but that would seem to be outside our control (and thus make a manual bump of the app's asset version necessary).

/cc @senny @lucasmazza

Member

matthewd commented Apr 17, 2014

Well as you note, it was recently added (#110, #69), so just undoing it doesn't seem likely to be a solution.

Perhaps we should just omit it iff it's a Proc? That obviously does leave the possibility that the proc's body has changed... but that would seem to be outside our control (and thus make a manual bump of the app's asset version necessary).

/cc @senny @lucasmazza

@matthewd matthewd added the Bug label Apr 17, 2014

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Apr 17, 2014

Member

Yes. Ignoring it if it is a proc seems a better solution since we can't evaluate the proc because we don't have the path, request

Member

rafaelfranca commented Apr 17, 2014

Yes. Ignoring it if it is a proc seems a better solution since we can't evaluate the proc because we don't have the path, request

@pomnikita

This comment has been minimized.

Show comment
Hide comment
@pomnikita

pomnikita Apr 17, 2014

Contributor

Would not it be confusing? I mean to bump asset.version manually in one case and not to in other

Contributor

pomnikita commented Apr 17, 2014

Would not it be confusing? I mean to bump asset.version manually in one case and not to in other

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Apr 17, 2014

Member

I think it is a good convention. If you use a proc as assets hosts Rails can't handle your case and you need to do yourself.

Member

rafaelfranca commented Apr 17, 2014

I think it is a good convention. If you use a proc as assets hosts Rails can't handle your case and you need to do yourself.

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Apr 17, 2014

Member

Yeah, I think it's pretty natural: if you're changing config settings, we can work it out automatically... if you're changing code [that can affect compiled assets], you bump the version.

This just happens to be a case where the code in question gets stored in the config.

Member

matthewd commented Apr 17, 2014

Yeah, I think it's pretty natural: if you're changing config settings, we can work it out automatically... if you're changing code [that can affect compiled assets], you bump the version.

This just happens to be a case where the code in question gets stored in the config.

@lucasmazza

This comment has been minimized.

Show comment
Hide comment
@lucasmazza

lucasmazza Apr 17, 2014

Member

👍 for ignoring, my bad for not handling this case back on the original PR.

Member

lucasmazza commented Apr 17, 2014

👍 for ignoring, my bad for not handling this case back on the original PR.

@scottjacobsen

This comment has been minimized.

Show comment
Hide comment

👍

@dnagir

This comment has been minimized.

Show comment
Hide comment
@dnagir

dnagir Jul 1, 2014

I'm surprised that 'ignoring' became prevalent resolution to this issue.

If the tool doesn't support something it should be raising an error with the description.
Otherwise people will be beating their heads up the wall trying to understand what is going on.

In this case I think an error message trying to precompile the assets would be a better option ("You are using asset_host with a proc. Because sprockets-rails doesn't know how to handle this we suggest you to use a static value instead.")

I am just asking you to consider alternative options as well because the "ignore" behaviour is inconsistent with the expectations of a common Rails developer (IMO):

  1. Raise error with a descriptive message (obvious what to do if it fails).
  2. Evaluate block passing nils (easily tracable when it fails).

dnagir commented Jul 1, 2014

I'm surprised that 'ignoring' became prevalent resolution to this issue.

If the tool doesn't support something it should be raising an error with the description.
Otherwise people will be beating their heads up the wall trying to understand what is going on.

In this case I think an error message trying to precompile the assets would be a better option ("You are using asset_host with a proc. Because sprockets-rails doesn't know how to handle this we suggest you to use a static value instead.")

I am just asking you to consider alternative options as well because the "ignore" behaviour is inconsistent with the expectations of a common Rails developer (IMO):

  1. Raise error with a descriptive message (obvious what to do if it fails).
  2. Evaluate block passing nils (easily tracable when it fails).
@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Jul 1, 2014

Member

@dnagir err, we support a proc... we just can't automatically detect that it's changed and cause a recompile. So we ignore the possibility that it's changed, with the justification that it's just like any other code change: you're obliged to bump the asset version yourself. Whereas given a string, we can easily determine whether it's changed, and force a recompile automatically.

Member

matthewd commented Jul 1, 2014

@dnagir err, we support a proc... we just can't automatically detect that it's changed and cause a recompile. So we ignore the possibility that it's changed, with the justification that it's just like any other code change: you're obliged to bump the asset version yourself. Whereas given a string, we can easily determine whether it's changed, and force a recompile automatically.

@dnagir

This comment has been minimized.

Show comment
Hide comment
@dnagir

dnagir Jul 2, 2014

@matthewd but this issue is happening when the proc is never being changed.

dnagir commented Jul 2, 2014

@matthewd but this issue is happening when the proc is never being changed.

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Jul 2, 2014

Member

Yes; this was a bug, and was fixed by #140.

Member

matthewd commented Jul 2, 2014

Yes; this was a bug, and was fixed by #140.

@rossta

This comment has been minimized.

Show comment
Hide comment
@rossta

rossta Jul 24, 2014

Thanks for this fix. The bug just bit us after upgrading to 2.1.3. We didn't catch it until deploying across multiple servers in production. I suspect many others could have the same issue before it's noticed. Plans for a new release?

rossta commented Jul 24, 2014

Thanks for this fix. The bug just bit us after upgrading to 2.1.3. We didn't catch it until deploying across multiple servers in production. I suspect many others could have the same issue before it's noticed. Plans for a new release?

@masone

This comment has been minimized.

Show comment
Hide comment
@masone

masone Jul 29, 2014

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

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 :)

@rsslldnphy

This comment has been minimized.

Show comment
Hide comment
@rsslldnphy

rsslldnphy Aug 18, 2014

Any plans for this to be released yet?

Any plans for this to be released yet?

@tillvollmer

This comment has been minimized.

Show comment
Hide comment
@tillvollmer

tillvollmer Sep 11, 2014

We have the same problem here, any plans to release it ?
We use something like this and get different digests:

config.asset_host = Proc.new { |source, request = nil |
"//cdnl#{Digest::MD5.hexdigest(source).to_i(16) % 6 + 1}.domain.com"
}

We have the same problem here, any plans to release it ?
We use something like this and get different digests:

config.asset_host = Proc.new { |source, request = nil |
"//cdnl#{Digest::MD5.hexdigest(source).to_i(16) % 6 + 1}.domain.com"
}

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 11, 2014

Member

It was already released.

Member

rafaelfranca commented Sep 11, 2014

It was already released.

@tillvollmer

This comment has been minimized.

Show comment
Hide comment
@tillvollmer

tillvollmer Sep 12, 2014

Thanks. Update helped !

Thanks. Update helped !

@schneems schneems referenced this issue in heroku/heroku-buildpack-ruby Sep 19, 2014

Closed

Rails 4 Asset Caching Doesn't Clean Old Assets #123

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment