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

Default Instance Storage #279

Merged
merged 14 commits into from Jul 26, 2017

Conversation

Projects
None yet
1 participant
@jnunemaker
Copy link
Owner

commented Jul 24, 2017

This adds a tidy little spot to configure the default flipper instance. This has long been a thing people have to think about to use flipper. Rarely do people use multiple flipper instances in the same app, and if they want to they still can, but at least now it is wrapped up for most people.

# these requires and the configure could sit in an initializer
require 'flipper'
require 'flipper/adapters/memory'

Flipper.configure do |config|
  config.default { Flipper.new(Flipper::Adapters::Memory.new) }
end

puts Flipper.enabled?(:search)
Flipper.enable(:search)
puts Flipper.enabled?(:search)
Flipper.disable(:search)

default is configured with a block. This makes it so it can either return a single instance no matter what or generate an instance safe for a thread, if necessary. A top level method Flipper.instance exists to memoize an instance of the default per thread.

module Flipper
  def instance
    Thread.current[:flipper_instance] ||= configuration.default
  end
end

Each thread should get its own flipper instance with this, which should keep things thread safe without much work. This seemed easier than synchronizing everywhere. Since Flipper::DSL instances are light, it seemed like a fine trade off. Let me know if you have any different thoughts.

All of the DSL methods (save group which is already top level in Flipper module) are now delegated to Flipper.instance. This makes it so instead of doing:

Flipper.instance.enabled?(:search)

You can do:

Flipper.enabled?(:search)

...which is shorter and easier to remember. The list of methods delegated at this time is here.

I'll let this sit for a day or so. Please drop any comments. If I've already merged, feel free to continue to drop comments and I'll follow up with subsequent PRs to address.

fixes #243
xref #261

jnunemaker added some commits Jul 23, 2017

Move default adapter to configuration of default instance
Feels like time to finally add some configuration. This makes it easy to
setup the default instance per thread using Flipper.configure.
Make flipper raise if default instance not set when attempting to use it
This feels better than defaulting to memory adapter which feels
unexpected. I'd rather see an error pop early with instructions on how
to fix.
Shorten default_instance to default
Default feels like enough and instance felt repetitive since the return
value is a DSL instance.
Disable the module function rubo cop
The reason is that module_function isn't delegating the def_delegators
methods and I don't want to figure it out right now. Happy to change it
if someone looks into it at some point.

@jnunemaker jnunemaker self-assigned this Jul 24, 2017

@jnunemaker jnunemaker merged commit 7095df8 into master Jul 26, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jnunemaker jnunemaker deleted the default branch Jul 26, 2017

@xaviershay xaviershay referenced this pull request Jun 6, 2019

Open

Thread safety? #261

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.