-
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); |
| }, | ||
| }, | ||
| basisReceived, | ||
| expect.any(Function), |
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.
Bug: New conditional logic in callback lacks test coverage
The tests were changed from verifying that basisReceived is passed directly to applyChanges to using expect.any(Function), but there's no test that verifies the new conditional behavior. The key new logic (if (payload.state !== '') determines whether basisReceived() is called) is not covered by any test. The tests don't verify that basisReceived is actually invoked when state is non-empty, nor that it's skipped when state is empty (for file data initializer scenarios). This means the core fix described in the PR could regress without test detection.
Requirements
Describe the solution you've provided
This PR will:
Note
Gate FDv2 basis readiness to only trigger when a non-empty state is present; update tests accordingly and add @experimental docs plus example for custom data source options.
src/data_sources/createPayloadListenerFDv2.ts, wrap theapplyChangesreadiness callback to invokebasisReceived()only whenpayload.stateis non-empty, aligning initializer readiness with valid selector presence.__tests__/data_sources/createPayloadListenersFDv2.test.tsto usetoHaveBeenCalledWithand expect the thirdapplyChangesarg asexpect.any(Function); adjust logger expectations.src/api/options/LDDataSystemOptions.ts, markCustomDataSourceOptionsas@experimentaland add a concise example demonstrating initializers and synchronizers configuration.Written by Cursor Bugbot for commit 935b994. This will update automatically on new commits. Configure here.