sstephenson / sprockets

Rack-based asset packaging system
2.55k stars 24 forks source link

Processor Cache Keys #672

Closed josh closed 9 years ago

josh commented 9 years ago

Implementation for #671.

Processors (engines or transformers) MAY define a method called cache_key that returns a static cache key identifying the state of the function. (Method name maybe revised)

/cc @chancancode @elia

elia commented 9 years ago

Looks good, seems that I'll finally be able to remove this: https://github.com/opal/opal/blob/master/lib/opal/sprockets/cache_key_fix.rb :)

chancancode commented 9 years ago

Thanks for the quick turnaround! I will try to patch my sample app and ember rails with this to try it out!

— Sent from Mailbox

On Sun, Nov 30, 2014 at 2:27 AM, Elia Schito notifications@github.com wrote:

Looks good, seems that I'll finally be able to remove this: https://github.com/opal/opal/blob/master/lib/opal/sprockets/cache_key_fix.rb :)

Reply to this email directly or view it on GitHub: https://github.com/sstephenson/sprockets/pull/672#issuecomment-64981368

josh commented 9 years ago

@elia you were supposed to tell me about all the hacks you were resorting to! :wink:

elia commented 9 years ago

@josh lol, I guess #508 was my attempt and was meant to replace the hack :smile:

(:grin: now I'm tempted to leverage the fact that I have you and @chancancode listening to bring attention to the other hack I have in place to "fix" rails/rails#7587)

chancancode commented 9 years ago

@elia let's move that to the Rails ticket

josh commented 9 years ago

@elia :flushed: totally forgot to revisit that.

@chancancode just let me know when you confirm this would address all the sample problems. Then I'll try to cleanup this PR and get it merge into master. It should got out in a 3.0 beta shortly after.

chancancode commented 9 years ago

@josh I think the concept is working fine for my purpose, I have implemented here.

However, it doesn't seem to be fully working yet. When I changed VERSION and restarted the server, I am getting this:

ActionController::RoutingError (No route matches [GET] "/assets/zomg-17ee2d799c8cd6e2f20499feb34fef6e28152e103fc0ef363d8a25361e7a6dc6.js"):
  actionpack (4.2.0.beta4) lib/action_dispatch/middleware/debug_exceptions.rb:21:in `call'
  actionpack (4.2.0.beta4) lib/action_dispatch/middleware/show_exceptions.rb:30:in `call'
  railties (4.2.0.beta4) lib/rails/rack/logger.rb:38:in `call_app'
  railties (4.2.0.beta4) lib/rails/rack/logger.rb:20:in `block in call'
  activesupport (4.2.0.beta4) lib/active_support/tagged_logging.rb:68:in `block in tagged'
  activesupport (4.2.0.beta4) lib/active_support/tagged_logging.rb:26:in `tagged'
  activesupport (4.2.0.beta4) lib/active_support/tagged_logging.rb:68:in `tagged'
  railties (4.2.0.beta4) lib/rails/rack/logger.rb:20:in `call'
  actionpack (4.2.0.beta4) lib/action_dispatch/middleware/request_id.rb:21:in `call'
  rack (1.6.0.beta2) lib/rack/methodoverride.rb:22:in `call'
  rack (1.6.0.beta2) lib/rack/runtime.rb:18:in `call'
  activesupport (4.2.0.beta4) lib/active_support/cache/strategy/local_cache_middleware.rb:28:in `call'
  rack (1.6.0.beta2) lib/rack/lock.rb:17:in `call'
  actionpack (4.2.0.beta4) lib/action_dispatch/middleware/static.rb:113:in `call'
  rack (1.6.0.beta2) lib/rack/sendfile.rb:113:in `call'
  railties (4.2.0.beta4) lib/rails/engine.rb:514:in `call'
  railties (4.2.0.beta4) lib/rails/application.rb:161:in `call'
  rack (1.6.0.beta2) lib/rack/lock.rb:17:in `call'
  rack (1.6.0.beta2) lib/rack/content_length.rb:15:in `call'
  rack (1.6.0.beta2) lib/rack/handler/webrick.rb:89:in `service'
  /Users/godfrey/.rvm/rubies/ruby-2.0.0-p353/lib/ruby/2.0.0/webrick/httpserver.rb:138:in `service'
  /Users/godfrey/.rvm/rubies/ruby-2.0.0-p353/lib/ruby/2.0.0/webrick/httpserver.rb:94:in `run'
  /Users/godfrey/.rvm/rubies/ruby-2.0.0-p353/lib/ruby/2.0.0/webrick/server.rb:295:in `block in start_thread'

I previously encountered another error about logical_path is not a thing, so I just hacked it and changed it to filename, so that could also be a factor?

Had to jump through a few hoops to test this on current rails/sprockets-rails :speak_no_evil: If you want to experiment yourself feel free to pull my test app. It's pretty minimal.

As far as ember-rails, that one was quite tricky because their setup is quite weird. I think they basically shell out to a JS runtime to compile the handlebars, so the compiler version isn't actually exposed to Ruby-land. I think that one would be solved more naturally by making the compiler file itself a dependency (via depend_on) of all the source files it touches.

josh commented 9 years ago

@chancancode give "dependable" another shot on Sprockets beta 6 and sprockets-rails master.

chancancode commented 9 years ago

@jsoh https://github.com/chancancode/dependable/commit/721e0e7cf58752f48f3e0cba3bfed486c6508e3a worked flawlessly :smile:

Great work! :heart: :+1:

josh commented 9 years ago

Hmm, so I think theres still a problem with this.

The processor cache key will only be incorporated into the direct asset using that processor. But wouldn't affect anything depending on it.

# foo.coffee
# would include CoffeeScript::VERSION

# app.js
#= require foo.coffee
# wouldn't since its just a .js file

So upgrading coffeescript wouldn't cause app.js to be stale.

We can't "just include the cache key" in app.js. It doesn't know how to compare it against anything. It'd have to know to call CoffeeScriptTemplate.version to compare with somehow.

I don't think this cache_key method on the proc is going to work. Theres just no way to serialize that reference so we know how to check it again.

josh commented 9 years ago

So heres maybe an idea.

A asset has a Set of "dependency uris" that are resolved to compute its cache key. Right now thats mainly "mtimes". Those could be expressed as mtime uris. Resolving mtime:///Users/josh/github/app/js/app.js would call a registered function that looked up the mtime of the file. Thats the basic case.

Then we could allow for cache uri extensions. In addition to mtimes, foo.coffee also depends on processor:coffee-script-version.

env.register_cache_uri 'processor:coffee-script-version', { CoffeeScript::VERSION }

This cache uris would be easy to serialize and persist between runs.

So to validate the cache of app.js, it would return a dependency set [ "mtime:///app.js", "mtime:///foo.coffee", "processor:coffee-script-version" ]. Resolving those URIs would be pretty fast.

Well, we'd want to ensure these cache uri resolvers are very fast, but it would allow people to do the stupid things they dream of like

env.register_cache_uri 'processor:erb-user-records', { Users.compute_cache_key_from_database_call }
josh commented 9 years ago

Sorry I so long to revisit this, but I do want to still sneak it in before the final 3.0.

So I did end up adding support for the dependency resolver thing but I still kinda want to add something like this as shorthand.

So something like this works on master.

Sprockets.register_dependency_resolver "opal-version" do
  Opal::VERSION
end

def call(input)
  # ...
  { data: data, dependencies: ["opal-version"] }
end

Its a little annoying for processors to aways have to register two things given that they usually will want to implement this.

The trick is still the serialization bit since theres no way to serialize functions to disk then restore them in place on load. So we need some sort of string indirection that serialized and can resolve us to our in memory check after a fresh boot.

We might have to add some sort of "name" symbol to register_processor that can be serialized along with the cached asset then used to lookup the processor. There might be some way to auto generate this based on the registration extension and other metadata. Still thinking that through.

chancancode commented 9 years ago

:heart: I'll try to find time to test this :soon:

josh commented 9 years ago

@chancancode rad!

class WhateverProcessor
  def self.cache_key; "1.0.0"; end
end

should work on master.