assets rake task refactoring work - the sequel #3225

Merged
merged 5 commits into from Oct 4, 2011

Projects

None yet

3 participants

@mjtko

From #3222:

@mjtko could you please provide a patch that is basically moving the write manifest to static compiler ?

Hi @josevalim et al!

Here is the new pull request.

Feel free to cherry-pick the changes you want - I've tested each of them in isolation but I think they're all reasonable enough.

Hope this patch has little enough cleverness for you! ;-)

Cheers,

Mark.

@josevalim josevalim merged commit 0ad5040 into rails:3-1-stable Oct 4, 2011
@josevalim
Ruby on Rails member

Ok, all merged, thanks. I will submit some changes right after to fix some small points I disagree. :)

@mjtko

Hehe, no problem. Hope that's the last we see of this task until, well... 3.1.2. ;-)

@josevalim
Ruby on Rails member

Actually, I did just one small change. Everything looked great. Thanks a lot for working on this with us, I think we have a very solid task right now cross fingers.

@mjtko

Thanks a lot for working on this with us

Any time, glad to be of help - seems that I found almost the right level of cleverness in the end! ;-)

I think we have a very solid task right now cross fingers.

Yes, it's all looking much better now! crosses eyes X-)

@thomasfedb

Just looking through the code and noticed this:

ruby_rake_task "assets:precompile:nondigest" if Rails.application.config.assets.digest

While I understand that this does work (after some time working though the code), it looks backwards. Perhaps something like the following would be better:

ruby_rake_task "assets:precompile:nondigest"
ruby_rake_task "assets:precompile:digest" if Rails.application.config.assets.digest
# order ensures that manifest will be overwritten if configured for digest

Just a thought.

@mjtko

Hi @thomasfedb,

Just working this through, I think it's worth stating how the code works at the moment. This current code current says:

  1. If the primary compilation is under a digest = true configuration, then we should also explicitly compile nondigest assets.
  2. If the primary compilation is under a digest = false configuration, then we've already compiled nondigest assets so we don't need to explicitly compile them.

My interpretation is that your suggestion is:

  1. Non-digest assets are always explicitly compiled.
  2. If the primary compilation is under a digest = true configuration, then we should explicitly compile digest assets.

While I agree that the condition now looks more sensible, the downsides are:

  1. We have to change more code, so a manifest file is always generated (probably not too much hassle :)), even if the manifest file is going to be overwritten.
  2. The digest compilation task would need to explicitly set digest = true.
  3. More re-invocations of the rake task when not necessary - if digest = false we additionally reinvoke once for nondigest. If digest = true we reinvoke additionally once for digest. Under the current code no reinvocation is necessary for the primary compilation run.

In short, while I agree with you to some extent, I'm not sure that the gains outweigh the costs. Would you be satisfied with a comment explaining why the logic looks inverted for the nondigest reinvocation? :) If not, I suggest we don't patch until after 3.1.1 is out of the door. :-)

@thomasfedb

Hey @mjtko,

Thanks for making sure we all understand the code execution.

I think the code is fine to use and release, a comment would be a bonus but not really necessary, once you read though it all you can work it out. Though I think for clarity sake that this could be tidied up for a future release.

I'd like to see some opinions of a few other rails devs on our differing ideas of how the logic should go. I'm happy to write a patch for my logic if you think that would be the best way to present the alternatives.

Cheers.

@mjtko

Hi!

Yup, there's certainly potential for more tidying. :-)

I would like to avoid too many rake re-invocations though - at the moment I'm able to get all my assets compiled in one invocation (I don't need the undigested assets) with:

rake assets:precompile:primary RAILS_ENV=production RAILS_GROUPS=assets

IMO, we should be shooting for as few invocations of rake as possible as it just slows things down otherwise.

@thomasfedb

Agreed. Is it possible to modify the Sprockets compiler (which I believe is the root of all this trouble) so that we don't need new evocations to be able to reset it?

Additionally what is the reason that we are forcing undigested assets to be generated?

@mjtko

Is it possible to modify the Sprockets compiler (which I believe is the root of all this trouble) so that we don't need new evocations to be able to reset it?

Possibly, but you'd have to dig into the sprockets code base (not the StaticCompiler task helper used by the assets rake task) to disable the caching mechanism during compilation or something.

The other reason for re-invocation is to ensure that the RAILS_GROUPS and RAILS_ENV environment variables are set up correctly for asset compilation. No real way round this that I've been able to come up with yet.

Additionally what is the reason that we are forcing undigested assets to be generated?

These are generated for use by static pages (error pages for example) or in emails. Their use is somewhat of an edge case (IMHO) but necessary when you're in a situation where you have no idea what the digested name is.

There has been some previous discussion about how best to deal with this use case - there's a middleware that deals with it for example - but the simplest way to prevent the stack from being entered at all is to have them generated en masse at compilation time.

(As an aside: I personally favour a web server URL rewriting configuration for dealing with this, but I understand that this is less than simple to get up and running and Rails can't really help you out with your web server configuration! :-)).

@thomasfedb

Possibly, but you'd have to dig into the sprockets code base (not the StaticCompiler task helper used by the assets rake task) to disable the caching mechanism during compilation or something.

This doesn't really sound like my cup of tea. How do you think the sprocket guys would feel about a request to expose an option?

The other reason for re-invocation is to ensure that the RAILS_GROUPS and RAILS_ENV environment variables are set up correctly for asset compilation. No real way round this that I've been able to come up with yet.

Would it be possible to manually load the production environment config to feed it into sprockets? I persoanlly have no idea what RAILS_GROUPS is, so I'll look into this.

These are generated for use by static pages (error pages for example) or in emails. Their use is somewhat of an edge case (IMHO) but necessary when you're in a situation where you have no idea what the digested name is.

Good point. Personally I would simply expose a rake task assets:precompile:nondigest so people needing this can run manually.

@mjtko

Possibly, but you'd have to dig into the sprockets code base (not the StaticCompiler task helper used by the assets rake task) to disable the caching mechanism during compilation or something.

This doesn't really sound like my cup of tea. How do you think the sprocket guys would feel about a request to expose an option?

I'm pretty sure it could be done. /cc @josh ...Any thoughts?

Would it be possible to manually load the production environment config to feed it into sprockets? I persoanlly have no idea what RAILS_GROUPS is, so I'll look into this.

The issue is that Rails must be initialized correctly with the correct bundler groups and initializers having fired. That isn't the default behaviour - Rails will default to development initialization. The reinvocation ensures that the default behaviour for asset compilation is sensible (ie. production environment and having loaded the assets bundle group).

Personally I would simply expose a rake task assets:precompile:nondigest so people needing this can run manually.

That task already exists, as does its compliment assets:precompile:primary. My opinion is that assets:precompile should provide the most coverage of all use cases, which is why it includes the nondigest compilation by default.

Perhaps adding more documentation to the assets guide about the various options is the way forward here. :)

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