chore: adds LDTransactionalFeatureStore, LDTransactionalDataSourceUpdates, and FDv2 DataSource impls. #833
Conversation
|
@launchdarkly/js-sdk-common size report |
|
@launchdarkly/js-client-sdk size report |
|
@launchdarkly/js-client-sdk-common size report |
tanderson-ld
force-pushed
the
ta/fdv2-code-move-part2
branch
from
76369ec to
eb45ed6
Compare
packages/shared/sdk-server/src/api/subsystems/LDFeatureStore.ts
Outdated
Show resolved
Hide resolved
packages/shared/sdk-server/src/api/subsystems/LDDataSourceUpdates.ts
Outdated
Show resolved
Hide resolved
tanderson-ld
changed the title
tanderson-ld
changed the title
…ates, and FDv2 DataSource impls.
tanderson-ld
force-pushed
the
ta/fdv2-code-move-part2
branch
from
e3f5ac5 to
b0d426c
Compare
There was a problem hiding this comment.
I have some concern that ordering is much more important now, so we may need to validate when callbacks can happen and when they can be applied.
In the old stuff you would have a stream and you know you will get puts and then later patches and the way the event source works we really aren't going to break that. Polling included the entire payload, so even http requests completing out of order wasn't a problem.
Now data could come from many sources that are less coordinated.
| import Requestor from '../../src/data_sources/Requestor'; | ||
| import TestLogger, { LogLevel } from '../Logger'; | ||
|
|
||
| describe('given an event processor', () => { |
There was a problem hiding this comment.
| describe('given an event processor', () => { | |
| describe('given a polling processor', () => { |
| requestor.requestAllData = jest.fn((cb) => cb(undefined, jsonData)); | ||
|
|
||
| processor.start(mockDataCallback, mockStatusCallback); | ||
| setTimeout(() => { |
There was a problem hiding this comment.
I would instead set this up to await a couple calls, then let the test timeout indicate failure. Potentially using the async queue and awaiting a couple take calls. This gives it a total of 5 seconds to happen, the test timeout, but doesn't cause it to require at least 500ms. Also this test may flake under high load as is.
| basis: boolean, | ||
| data: LDFeatureStoreDataStorage, | ||
| callback: () => void, | ||
| initMetadata?: InitMetadata, |
There was a problem hiding this comment.
Did you need the type alias for some reason?
| }); | ||
| } | ||
|
|
||
| stop() { |
There was a problem hiding this comment.
I am not sure about this. Do all the callbacks get disconnected from this, in the situation we don't want to use it anymore?
It is a little different for client-side, but I saw this assumption is causing problems in the client-side SDK where a poll completes after stop and the data propagates.
In this case maybe the unintended propagation would be fine? But maybe it could cause an unexpected basis to apply a delta to?
| const startTime = Date.now(); | ||
| this._logger?.debug('Polling LaunchDarkly for feature flag updates'); | ||
| this._requestor.requestAllData((err, body) => { | ||
| const elapsed = Date.now() - startTime; |
There was a problem hiding this comment.
We likely want a _stopped check inside the callback, so we do not apply data/status from a stopped data source.
| ]; | ||
| } | ||
|
|
||
| function computeDependencies(namespace: string, item: LDFeatureStoreItem) { |
There was a problem hiding this comment.
This code was just moved, correct?
Requirements
This PR moves changes for FDv2 from the temporary holding branch to main. All picked code has been previously reviewed and was picked from the temporary holding branch using file pick commands.
Resolved conflicts with @abarker-launchdarkly's recent initMetadata changes. Please verify those conflict resolutions @abarker-launchdarkly .