feedback for new cache implementation about jbuilder HOT 7 CLOSED

@rolftimmermans, can you look at the blankslate issue?

@rgarver, can you check the caching problem?

from jbuilder.

I can replicate this result. Not sure why the tests aren't catching this.

from jbuilder.

Ah, @fluxsaas in order for this to work you need to pass in a context for the cache! call. Eg:

  # index.json.jbuilder
  json.cache! "users" do |json|
    json.users @users do |json, user|
      json.id user.id
    end
  end

The reason it was failing for you is because you were attaching the users to the json context outside the cache! scope. Try this and let me know how it works.

from jbuilder.

Oh, doh. I missed that part too.

from jbuilder.

Although I agree that an explicit argument to the block is how the current API works, I think it might worth considering supporting the other variant too. Currently the json block arguments are always redundant and only there for backwards API compatibility. You'll always get the same instance of Jbuilder.

To me it makes sense to implement cache! in the same new way. It is simpler, faster and leads to fewer bugs. For example, cache! currently resets the key formatter, which is not what may be expected. I can try to write a patch later.

from jbuilder.

Do look into the complications of not requiring the yield. We definitely still want to use the json yield at the top-level to tie all calls off, but maybe we don't need it in the subsequent blocks.

On Sep 25, 2012, at 10:32 AM, Rolf Timmermans wrote:

Although I agree that an explicit argument to the block is how the current API works, I think it might worth considering supporting the other variant too. Currently the json block arguments are always redundant and only there for backwards API compatibility. It would make sense to implement cache! in the same new way. It is simpler and leads to fewer bugs. For example, cache! currently resets the key formatting to default, which is not what may be expected. I can try to write a patch later.

—
Reply to this email directly or view it on GitHub.

David Heinemeier Hansson

from jbuilder.

yes, works fine. i guess i was used to the rails cache method and missed the scope...

from jbuilder.