Skip to content
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

! Fix class including a module with Memoizable raises error #62

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

PikachuEXE
Copy link

My project uses "concern module pattern"

e.g. multiple controller classes including concern modules with instance methods cached

I am switching from memoist since it's officially unmaintained

@flash-gordon
Copy link
Member

Excuse my ignorance but what is "concern module pattern"? :) Why is this patch needed? At first glance, I don't feel comfortable with having a workaround for a misused ruby API.

@PikachuEXE
Copy link
Author

PikachuEXE commented Oct 23, 2021

A brief example:

require "dry/core/memoizable"

module WithBotDetection
  include Dry::Core::Memoizable

  def self.included(base)
    # Do something custom
  end

  def is_bot?(something = false)
    # ...
  end
  memoize :is_bot?
end

class Controller1
  include WithBotDetection
end

class Controller2
  include WithBotDetection
end

Controller1.new.is_bot? # => NoMethodError: undefined method `key?' for nil:NilClass

Due to #61 only happens to methods with arguments

More about "concern module pattern":
https://api.rubyonrails.org/classes/ActiveSupport/Concern.html

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.

2 participants