Add methods to allow generation of critical CSS for a specific route …

[taranda]

…and to allow removal of specific routes from the CSS cache.

This modification exposes some methods allowing the developer to generate specific routes dynamically. It also allows the developer to clear routes from the cache. With these added tools, developers can dynamically generate critical CSS without relying on hardcoded routes in critical_path_css.yml.

This could solve this first issue in the backlog. For example, a user could add all of the URLs for a products resource with the following code snipet:

Product.all.each do |product|
  CriticalPathCss.generate "/product/#{product.id}"
end

It does not allow the user to add controller routes to the config yml, but it gives the developer the tools he/she needs to add all of the routes for a resource to the cache.

It also allows developers to dynamically generate critical CSS for their whole app without having to remember to hard code all of routes in the configuration yml. Here is a sample implementation of dynamically generated critical CSS

This pull request is completely backward compatible with previous versions.

I look forward to your feedback.

[michael-misshore]

On my first walk-through, this is awesome!!! I'll run this by my team asap, and then we'll merge this in.

[michael-misshore]

After having some time to review, I really am excited about the addition and it should provide great value to the project. Here are a few suggestions:

While convenient, I think we should remove arguments that allow for silent method failure, i.e. routes=nil. I think there's benefit of an error being raised if the developer used/uses the wrong method. Maybe something like:

def self.generate(route)
  Rails.cache.write(route, CssFetcher.new.fetch_route(route), namespace: CACHE_NAMESPACE)
end

def self.clear(route)
  Rails.cache.delete(route, namespace: CACHE_NAMESPACE)
end

def self.clear_matched(routes)
  Rails.cache.delete_matched(routes,namespace: CACHE_NAMESPACE)
end

def self.clear_all
  clear_matched('*')
end

We'll probably need to update anything currently referencing generate to generate_all in this case.

It may be helpful to include _cache to ensure the method's behavior is clear.

Let me know what you think!

[taranda]

Good suggestions. How important is it to be backward compatible? Removing the default route=nil from the generate method would break backward compatibility. We could do something like this:

def generate_for (route)
  Rails.cache.write(route, CssFetcher.new.fetch_route(route), namespace: CACHE_NAMESPACE)
end

We would leave the existing generate method unchanged. This preserves backward compatibility while removing the default value in the method's argument.

I like the changes to the clear methods. I think the behavior of clear is clear enough (no pun intended). The user need not be concerned with the fact that the CSS is stored in the cache. He/she need only be concerned with the concept that the critical CSS for a given route is being cleared and will no longer be available to fetch without being generated again.

Thoughts?

[michael-misshore]

Hmmmm, It's tough to gauge how many folks are actually using this, but given the issues reported and contributions made, I think we should maintain backwards compatibility until a larger change is requested/introduced. With that being said, I like the idea using generate_for here.

On a side note, I plan to attempt to move away from Penthouse to Critical CSS. This change should produce a major/minor version bump. I'm thinking in the short-term a method_alias could be added for generate_all/generate, while providing a deprecation notice for generate in preparation for the mentioned major/minor version bump. This is all initial thought, however...

Regarding the clear method name changes, I'm fine leaving it as is. I think you make a good point regarding the user not needing to care about how the CSS is being stored.

Once these changes have been completed, we should be good to go to get this PR merged in. Thanks again for the contribution and let me know if you have any other thoughts/feedback.

[taranda]

Now that I think about this some more, I really like the idea of using generate (route) and generate_all. If I did not have to consider backward compatibility, that is exactly how I would design it. I know I'm the one who brought up backward compatibility in the first place, but you have the experience with this gem, how important do you think backward compatibility really is in this case?

[taranda]

I just force pushed an update that uses generate_for. I'm going with the backward compatible option. Cheers.

[michael-misshore]

Sorry for the delayed response. I meant to respond yesterday...

After some thought, let's go ahead with the generate(route) and generate_all route. The generate method is only used once in this gem (rake task) and because the method is static, it should be easy for users to update if they are calling this manually in their codebase.

With that being said, the version should be incremented to 0.3.0 because of the above.

Side note, I don't have a preference with generate vs. generate_for.

[taranda]

Okay. I like generate, so I will make a change and push it up today.

[taranda]

Sorry this took so long. I just pushed an update to make the changes we suggested. It looks like there are some merge conflicts. Would you like to pull them down and resolve them or would you like to take care of it?

[taranda]

I rebased and now we can merge without issue.

[taranda]

This should be ready to merge. Do you see any issues?

[michael-misshore]

@taranda Sorry for the delay. The work looks solid, but I want to test this locally first due to the absence of a test suite. I'd like to get this merged in by EOW, however.

[taranda]

No worries. Thanks.

[michael-misshore]

I found two quick things:

First, we need to add expires_in: nil to the generate method's cache write.

Second, Rails.cache.delete_matched actually expects a regular expression when giving a namespace: http://apidock.com/rails/ActiveSupport/Cache/Store/key_matcher. I was getting an error when running the clear_all rake task. This seemed to work for me:

def self.clear_all
  self.clear_matched(/.*/)
end

We may want to make that clear in the documentation as well if clear_matched is used straight up.

Likewise, I stumbled upon that delete_matched is not supported on Memcached, for what it's worth: http://apidock.com/rails/v4.2.7/ActiveSupport/Cache/Store/delete_matched. It maybe worth looking into a different implementation, but I think it's fine for now.

Once these two changes are in, I think this is ready to be merged in.

[taranda]

Hmm. I changes the clear_all method like you suggested and got an error with Redis. The Redis cache seems to require strings with wildcards and does not support regex. It seems the underlying cache implementation affects what we need to pass into delete_cache. Redis likes '*' and the cache implementation you used for testing likes /.*/.

What caching systems would you like to support?

Since Memcached is not supported already, I recommend supporting Redis.

We could remove the clear_all method entirely.

[michael-misshore]

Could we do something like Rails.cache.clear and pass it the namespace?

I'm fine with removing the method unless there's a huge need for it. I'd like to support as many options as possible.

[taranda]

I think Rails.cache.clear will clear the entire cache. At least it did with my experiments with a Redis cache. In other words, you cannot clear one namespace. It may be implementation dependent. We could remove the clear_all methods and simply add several options to the clear_all rake task that users can uncomment depending on their implementation.

I know Redis likes '*'. What cache implementation were you using that liked /.*/?

[michael-misshore]

Yeah, Rails.cache.clear by itself will definitely nuke everything. I just wasn't sure if there were any namespace type options to pass to it.

I'm fine with your solution of remove the clear_all methods and the commented options in the rake task. Once that's complete, I can get this merge in.

[taranda]

Those changes have been pushed up. You may want to retest.

[taranda]

How are things looking?

[michael-misshore]

Sorry for the delay. I think i'll have time to test this later today

[taranda]

No worries.