-
Notifications
You must be signed in to change notification settings - Fork 20
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
fix: Increment version for changing flagValues #317
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,22 +6,23 @@ import { | |
VoidFunction, | ||
} from '@launchdarkly/js-sdk-common'; | ||
|
||
import { DataKind, LDFeatureStore, LDFeatureStoreDataStorage } from '../api'; | ||
import { DataKind, LDFeatureStoreDataStorage } from '../api'; | ||
import { FileDataSourceOptions } from '../api/integrations'; | ||
import { LDDataSourceUpdates } from '../api/subsystems'; | ||
import { Flag } from '../evaluation/data/Flag'; | ||
import { processFlag, processSegment } from '../store/serialization'; | ||
import VersionedDataKinds from '../store/VersionedDataKinds'; | ||
import FileLoader from './FileLoader'; | ||
|
||
export type FileDataSourceErrorHandler = (err: LDFileDataSourceError) => void; | ||
|
||
function makeFlagWithValue(key: string, value: any): Flag { | ||
function makeFlagWithValue(key: string, value: any, version: number): Flag { | ||
return { | ||
key, | ||
on: true, | ||
fallthrough: { variation: 0 }, | ||
variations: [value], | ||
version: 1, | ||
version: version ?? 1, | ||
}; | ||
} | ||
|
||
|
@@ -42,7 +43,7 @@ export default class FileDataSource implements subsystem.LDStreamProcessor { | |
constructor( | ||
options: FileDataSourceOptions, | ||
filesystem: Filesystem, | ||
private readonly featureStore: LDFeatureStore, | ||
private readonly featureStore: LDDataSourceUpdates, | ||
private initSuccessHandler: VoidFunction = () => {}, | ||
private readonly errorHandler?: FileDataSourceErrorHandler, | ||
) { | ||
|
@@ -102,6 +103,7 @@ export default class FileDataSource implements subsystem.LDStreamProcessor { | |
|
||
private processFileData(fileData: { path: string; data: string }[]) { | ||
// Clear any existing data before re-populating it. | ||
const oldData = this.allData; | ||
this.allData = {}; | ||
|
||
// We let the parsers throw, and the caller can handle the rejection. | ||
|
@@ -117,7 +119,7 @@ export default class FileDataSource implements subsystem.LDStreamProcessor { | |
parsed = JSON.parse(fd.data); | ||
} | ||
|
||
this.processParsedData(parsed); | ||
this.processParsedData(parsed, oldData); | ||
}); | ||
|
||
this.featureStore.init(this.allData, () => { | ||
|
@@ -128,13 +130,22 @@ export default class FileDataSource implements subsystem.LDStreamProcessor { | |
}); | ||
} | ||
|
||
private processParsedData(parsed: any) { | ||
private processParsedData(parsed: any, oldData: LDFeatureStoreDataStorage) { | ||
Object.keys(parsed.flags || {}).forEach((key) => { | ||
processFlag(parsed.flags[key]); | ||
this.addItem(VersionedDataKinds.Features, parsed.flags[key]); | ||
}); | ||
Object.keys(parsed.flagValues || {}).forEach((key) => { | ||
const flag = makeFlagWithValue(key, parsed.flagValues[key]); | ||
const previousInstance = oldData[VersionedDataKinds.Features.namespace]?.[key]; | ||
let { version } = previousInstance ?? { version: 1 }; | ||
// If the data is different, then we want to increment the version. | ||
if ( | ||
previousInstance && | ||
JSON.stringify(parsed.flagValues[key]) !== JSON.stringify(previousInstance?.variations?.[0]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't deterministic (it usually will be, but sometimes the JSON properties may serialize in different orders). So we may want to do something different here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't recommend using FileDataSource in production, so this may be sufficient. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The different ordering is a concern, and the best way to deep compare is to use lodash's isEqual function. I'm reluctant to introduce dependency on lodash/is-equal, so if FileDataSource is only used in dev, I think this is ok for now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can do with this, and then change when we can. Most flags aren't structured as well. |
||
) { | ||
version += 1; | ||
} | ||
const flag = makeFlagWithValue(key, parsed.flagValues[key], version); | ||
processFlag(flag); | ||
this.addItem(VersionedDataKinds.Features, flag); | ||
}); | ||
|
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.
Is null coalescing needed here?
version
is a number and should not be null or undefined.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.
Interesting that linting doesn't catch this. It was leftover from an interim version. Thank you for noticing it.