-
Notifications
You must be signed in to change notification settings - Fork 6
[FME-12059] SDK_UPDATE with metadata #461
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: development
Are you sure you want to change the base?
Conversation
|
|
||
| function updateSegment(segmentName: string, noCache?: boolean, till?: number, fetchOnlyNew?: boolean, retries?: number): Promise<boolean> { | ||
| log.debug(`${LOG_PREFIX_SYNC_SEGMENTS}Processing segment ${segmentName}`); | ||
| const sincePromise = Promise.resolve(segments.getChangeNumber(segmentName)); |
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.
If metadata is only used inside the if condition, we should refactor it as:
if (readiness) {
const metadata: SdkUpdateMetadata = {
type: SdkUpdateMetadataKeys.SEGMENTS_UPDATE,
names: []
};
readiness.segments.emit(SDK_SEGMENTS_ARRIVED, metadata);
}
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.
Idem for splitChangesUpdater
| /** | ||
| * Metadata keys for SDK update events. | ||
| */ | ||
| export enum SdkUpdateMetadataKeys { |
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.
In my opinion this enum should be removed and string constants should be used instead, because the SdkUpdateMetadata type in splitio.d.ts already defines the type field as a string literal union.
So, you can remove the enum and define the following constants in the readiness/constants.ts file:
...
// SdkUpdateMetadata types:
const FLAGS_UPDATE = 'FLAGS_UPDATE';
const SEGMENTS_UPDATE = 'SEGMENTS_UPDATE'
and reuse them across the code.
Using string literal unions rather than enum or const enum is a recommended practice for several reasons. For example, the output built code is smaller, const enum is incompatible with some build setups, etc.
JavaScript commons library
What did you accomplish?
How do we test the changes introduced in this PR?
Extra Notes