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

Lazy refresh instead of refresh on set_repository #19

Closed
javidjamae opened this issue Oct 16, 2014 · 3 comments
Closed

Lazy refresh instead of refresh on set_repository #19

javidjamae opened this issue Oct 16, 2014 · 3 comments
Assignees

Comments

@javidjamae
Copy link
Contributor

In a Rails app, when I do an asset:precompile, my initializers are loaded. When I'm setting the RedisRepository, the lib/feature.rb will call a refresh! when set_repository is called:

https://github.com/mgsnova/feature/blob/master/lib/feature.rb#L41

The problem here is that it will try to access Redis on initialization, but when we're doing asset:precompile, we don't have REDIS environment variables set.

I can work around this in our app doing something like this in my initializer:

begin
  Redis.current.ping
  Feature.set_repository(Feature::Repository::RedisRepository.new("feature_toggles"))
rescue Redis::CannotConnectError
  STDERR.puts "WARN: Could not connect to redis. This is OK in asset:precompile, but not in the live environment"
end

But... I think the better solution would be to not do a forced refresh when the repository is set, but rather do it lazily when any of the Feature methods are called for the first time. Thoughts?

@mgsnova mgsnova self-assigned this Oct 20, 2014
@mgsnova
Copy link
Owner

mgsnova commented Oct 20, 2014

Thanks for this input - I had not thought about such problems. I think a lazy initialization would be better - I will check if the rest can handle this.

@javidjamae
Copy link
Contributor Author

If you're ok with this change, then I can put together a pull request for your review. I'd be happy to take a stab at doing #17 (auto-refresh) along with it.

@mgsnova
Copy link
Owner

mgsnova commented Oct 21, 2014

I already implemented this one. Feel free to send a PR for #17. Thanks for the input again

@mgsnova mgsnova closed this as completed Oct 21, 2014
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

No branches or pull requests

2 participants