Skip to content

Top level arrays and fragment caching #35

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

Closed
wants to merge 3 commits into from

Conversation

mbrandonw
Copy link

I've added a method to Jbuilder so that we can use the fragment caching pattern from ActionView (see issue #22), but along the way I also had to make a change to array!.

My change to array! allows one to simply set the array directly, similar to how set! allows one to set a key/value pair directly:

json.array! [1,2,3]  # => "[1,2,3]"

This change is pretty useful by itself. But, with this I created a method cache! that has the same signature as the helper cache in ActionView and allows the following pattern:

json.cache! ['v1', @person], :expires_in => 10.minutes do |json|
  json.extract! @person, :name, :age
end

There is still some work to be done (write tests, update readme, and right now I'm just using Rails.cache for the cache store so i guess that needs to be changed), but is this something people are interested in pulling?

@maletor
Copy link

maletor commented Apr 20, 2012

Can't be sure of exact reproduction steps or if valid bug, but rendering a partial within a partial that calls Paperclip::Attachment#expiring_url throws an error in dalli.

Marshalling error for key 'jbuilder/0.303715376742858': singleton can't be dumped
You are trying to cache a Ruby object which cannot be serialized to memcached.
/Users/maletor/.rvm/gems/ree-1.8.7-2012.02/gems/dalli-1.1.5/lib/dalli/server.rb:266:in `dump'

@rgarver
Copy link
Contributor

rgarver commented May 2, 2012

Is there are reason not to bypass the double serialization from the cache to jbuilder and then again to output? Can you drop the raw string in a wrapper class that has a to_json that returns the string unaltered and attach it to the json at that point?

I haven't had a chance to test this approach out so I'd genuinely like to know if this works.

@mbrandonw
Copy link
Author

Good point. That would require some deep changes to jbuilder. I don't have the time to explore that, and I doubt the performance gains would be worth it. I've been using this caching for a few weeks now and pretty happy with it.

@rgarver
Copy link
Contributor

rgarver commented May 3, 2012

Got it. I may take a stab at it sometime this week.

@eviltrout
Copy link

I'm very interested in this patch! Let me know if I can help out in any way (tests, documentation, etc.)

@maletor
Copy link

maletor commented May 18, 2012

This should write to Rails.logger like it does in ERB.

Write fragment 'blah'
Read fragment 'blah'

@alanpca
Copy link

alanpca commented May 31, 2012

@eviltrout sounds like tests and documentation is necessary. I just stumbled upon this today and am pretty interested in some of the problems outlined above.

@maletor
Copy link

maletor commented Jun 4, 2012

#35 (comment)

Pretty sure this was a result of #34.

@maletor
Copy link

maletor commented Jun 4, 2012

@alanpca is right. Some tests, documentation and probably an implementation of the Read/Write fragment log and this will be looking really good.

@rgarver did you ever get a chance?

@rgarver
Copy link
Contributor

rgarver commented Jun 6, 2012

@maletor I did make some progress on this, but didn't have enough time to really dig in to tests and docs. I just polished a corner case for my purposes this evening. My version lives here:

https://github.com/rgarver/jbuilder/tree/feature/add-caching

@maletor
Copy link

maletor commented Jun 14, 2012

@rgarver looks really good. Might need a test for trying to de/serialize something with a Proc in it. What do you think?

@dhh
Copy link
Member

dhh commented Aug 8, 2012

I like this. Please update against master and we can merge.

@rgarver
Copy link
Contributor

rgarver commented Aug 10, 2012

I'm in the middle of a long-distance move so sorry for the delayed response. I'll get my branch updated to master as soon as I can post a new pull request.

@maletor
Copy link

maletor commented Aug 10, 2012

@rgarver it looks like it's a fast forward for your branch.

@rgarver rgarver mentioned this pull request Aug 10, 2012
@rgarver
Copy link
Contributor

rgarver commented Aug 10, 2012

See pull request #51 has my branch merged up with the latest master.

@dhh dhh closed this Sep 26, 2012
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.

6 participants