chore: Use FDv1 DataSystem in the ldclient #345
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| # @return [Array<EvaluationDetail, [LaunchDarkly::Impl::Model::FeatureFlag, nil], [String, nil]>] | ||
| # | ||
| def variation_with_flag(key, context, default) | ||
| private def variation_with_flag(key, context, default) |
There was a problem hiding this comment.
The file used a mix of per method private and all methods after the declaration on line 717 of the original file. Moving to defining per method.
| # so we're not connecting to any LD services). | ||
| if !config.offline? && sdk_key.nil? | ||
| # SDK key can be nil only if using LDD or custom data source with events disabled | ||
| if config.send_events || (!config.use_ldd? && config.data_source.nil?) |
There was a problem hiding this comment.
This conditional doesn't make sense to me.
The SDK key is required if:
- We are sending events, or
- We aren't in daemon mode AND we don't have a data source?
If we aren't in daemon mode, then we should have a data source, and that's when an SDK key is required. If there isn't a data source, what is the key for?
There was a problem hiding this comment.
I'll update the comments to clarify what is happening here. Essentially if we are using a custom data source (like FileData) we don't require an SDK key. If the datasource is nil, then we create a default data source (either polling or streaming) which require an sdk key.
| # | ||
| def initialized? | ||
| @config.offline? || @config.use_ldd? || @data_source.initialized? | ||
| @data_system.data_availability == @data_system.target_availability |
There was a problem hiding this comment.
Are we sure these lines are equivalent given our previous discussions about this? I can't remember exactly where we landed on that.
There was a problem hiding this comment.
They should be equivalent for FDv1. This will need to be altered a bit when we introduce FDv2.
- Offline returns and expects
DEFAULTS✅ - Use_Ldd returns and expects
REFRESHED(because it uses null processor) ✅ - If the data_source.Initialized is true the data_system returns
REFRESHEDandCACHEDif false, but we expectREFRESHED✅
lib/ldclient-rb/ldclient.rb
Outdated
| # @return [LaunchDarkly::Interfaces::DataStore::StatusProvider] | ||
| # | ||
| attr_reader :data_store_status_provider | ||
| def data_store_status_provider |
There was a problem hiding this comment.
You could also make these delegators if you wanted to tighten the syntax up some. It would be something like:
require 'forwardable'
# ...
extend Forwardable
def_delegators :@data_system, :data_store_status_provider, :data_source_status_provider
Note
Adopt FDv1 data system in client
Impl::DataSystem::FDv1; FDv1 wraps the originalfeature_storeonce (avoids nested wrappers), injectsdata_source_update_sink, exposesstore,flag_change_broadcaster, and status providers, and cleans up viastop(includingstore_wrapper.stop).data_store_status_provideranddata_source_status_providerfrom LDClient to the data system; addflushdelegator to the event processor; refine SDK key nil validation for offline/LDD/custom sources.initialized?now comparesdata_availabilityvstarget_availability; all reads switched todata_system.storein evaluations andall_flags_state.Tests
spec/impl/data_system/fdv1_spec.rbcovering store wrapping (no nested wrappers), processor selection (streaming/polling/offline/LDD), custom data source factory/instance handling, start/stop idempotency, availability semantics, diagnostic accumulator integration, and provider/broadcaster access.Written by Cursor Bugbot for commit 59ae3c0. This will update automatically on new commits. Configure here.