-
Notifications
You must be signed in to change notification settings - Fork 25
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
Memowise v1.2.0 regression with Rails concerns #227
Comments
Thanks @pomartel ! I failing test would be amazing |
After doing a few more tests, it seems the regression is related to Rails concerns. If I take the memoized method out of the concern and put it directly in my model, then it works as expected. I don't think I can write a test for that since Memowise is not Rails specific. If I rollback to Memowise v1.1.0 then the memoized concern method works fine. Is there anything I could provide to help with this? |
What I'd recommend is that we find a minimal test reproduction. Is it possible for you to describe the steps to trigger this in a Rails app? We can then reproduce, and work towards a minimal test case possible just using ActiveConcern? |
I spent quite a few hours trying to come up with a minimal test case and I'm really puzzled. I think it comes down to the way this all integrates in my app and the fact that I was including Memowise twice: class MyModel < ApplicationRecord
prepend MemoWise
include MyConcern
end
# THIS SEEMS TO CAUSE A REGRESSION WITH MEMOWISE 1.2
concern :MyConcern do
prepend MemoWise
def method_to_memowise
true
end
memo_wise :method_to_memowise
end
# THIS WORKS
concern :MyConcern do
included do
memo_wise :method_to_memowise
end
def method_to_memowise
true
end
end The second one works fine so I will close the bug report as it looks to be something related to my code implementation. |
Hi @pomartel! Thanks for digging into this! I'm glad you got something that works for you but I'm still interested in digging into this issue to see if there's a broader fix we can make in this gem. I'm having trouble reproducing what you're seeing though; I couldn't get the exact syntax you had to run but I tried this: require "memo_wise"
require "active_support/concern"
module MyConcern
extend ActiveSupport::Concern
included do
prepend MemoWise
def method_to_memowise
true
end
memo_wise :method_to_memowise
end
end
class MyModel
prepend MemoWise
include MyConcern
end
puts MyModel.new.method_to_memowise When I run this with |
We also just released v1.3.0. Can you see if you still see errors in that version? |
Ah! Yes, v1.3.0 did fix it! I am glad it did because I was trying hard to create a minimal Rails project to reproduce the issue and I just could not reproduce it. The bug only manifested in the app and not in the console. I was getting a nil value instead of a boolean. I saw in the recent commits something about thread safety (b807114). I think that fixed it! Thanks for following up. I think this can be closed now. |
So glad to hear it! I think this was probably related to the inheritance issues we fixed in #229 |
I just upgraded from v1.1.0 to v1.2.0 and there seems to be a regression with methods returning booleans. I will see tomorrow if I can write a failing test.
The text was updated successfully, but these errors were encountered: