-
Notifications
You must be signed in to change notification settings - Fork 31
fix: FDv2 initializer readiness #1017
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
base: main
Are you sure you want to change the base?
Conversation
|
@launchdarkly/browser size report |
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/js-client-sdk-common size report |
|
@launchdarkly/js-client-sdk size report |
packages/shared/sdk-server/src/data_sources/createPayloadListenerFDv2.ts
Show resolved
Hide resolved
packages/shared/sdk-server/src/data_sources/createPayloadListenerFDv2.ts
Show resolved
Hide resolved
packages/shared/sdk-server/src/data_sources/createPayloadListenerFDv2.ts
Outdated
Show resolved
Hide resolved
| // NOTE: this is a hack right now. The only condition that we will consider a valid basis | ||
| // is when there is a valid selector. Currently, the only data source that does not have a | ||
| // valid selector is the file data initializer, which will have a blank selector. | ||
| basisReceived(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would the FDv2 fallback also not have a selector? I may just not know how that part works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my understanding here, the basis can be consumed without a selector, however that would not be considered a successful initialization from this particular initializer. I think this function might not be named correctly here as from the invocation it is simply a success callback (
| payloadListener = createPayloadListener(dataSourceUpdates, logger, initSuccess); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree there is a bug in the code before this PR as even when payload.basis is false, basisReceived is called.
LDClientImpl is passing in a function called initSuccess. I think at that time I had thought getting a basis (independent of selector) was sufficient to consider it initialized. This would be my thinking at the time since I thought this code path was only hit when basis == true.
Since Casey's requirement is "Basis with a Payload Selector.", should this logic be updated to ensure basis is also true when selector is not null?
The name basisRecieved in basisReceived: VoidFunction = () => {} parameter may need to be adjusted.
packages/shared/sdk-server/__tests__/data_sources/createPayloadListenersFDv2.test.ts
Show resolved
Hide resolved
this commit also changes the initialization callback funciton to `initializedCallback` from `basisRecieved` to be more clear on what the callback is for.
Requirements
Describe the solution you've provided
This PR will:
Note
FDv2 now calls the initialization callback only when
payload.stateis non-empty, with tests added, and docs updated with an example forcustomdata source options.createPayloadListenerFDv2, replacebasisReceivedwithinitializedCallbackand invoke it only whenpayload.state !== ''duringapplyChanges.createPayloadListenersFDv2.test.tsto useinitializedCallbackand assert conditional invocation for non-empty vs empty state; keep existing behavior checks for basis/updates.CustomDataSourceOptionsinLDDataSystemOptions.tswith@experimentalnote and a usage example mirroring default standard options.Written by Cursor Bugbot for commit 38af523. This will update automatically on new commits. Configure here.