search

tansengming / stripe-rails

A Rails Engine for integrating with Stripe
MIT License
753 stars 123 forks source link

don't clear callbacks upon class unload if eager_load configured [fixes #155] #156

Closed alexagranov closed 5 years ago

tansengming commented 5 years ago

thanks for the PR @alexagranov . I am wondering if this would undo #137 , especially for folks who want to reload the class and also need it to be eager loaded.

Is it possible to reload the eager loaded classes within the block instead?

I'm on vacation right now but will be happy to look at it some more when I get back but feel free to explore.

alexagranov commented 5 years ago

thanks for the quick response @tansengming. I was thinking about that. During my testing I didn't see any redundant hooks called - which I thought #137 was trying to address.

I don't think this PR undoes #137 because if someone wants reloading, would they be using eager loading too?

Without this PR, it's just not possible to use the eager loading mechanism as #137 will clear the callbacks. Does eager loading + reloading make sense?

tansengming commented 5 years ago

if someone wants reloading, would they be using eager loading too?

Yup! If rephrasing it as "if someone eager loads, would they want to reload too?", it seems clear to me that it is something that folks expect.

I've tested 3 scenarios over at https://github.com/tansengming/stripe-rails-dummy/pull/2 . Basically it looks like:

  1. it fails exactly like you described on https://github.com/tansengming/stripe-rails/issues/155
  2. it fails in a different way when using this PR
  3. And I think I have a better fix which passes on https://github.com/tansengming/stripe-rails/pull/159

can you try out https://github.com/tansengming/stripe-rails/pull/159 and see if it works for you too? Thanks!

alexagranov commented 5 years ago

@tansengming thanks will do