-
Notifications
You must be signed in to change notification settings - Fork 31
feat: add FDv2 FileDataSource initializer #1010
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
Changes from 4 commits
66c80c7
678e42f
15d65d7
10363a9
b02abe9
b1e0123
718b11a
c42e4f4
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 |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| import FDv2ChangeSetBuilder from '../../../src/internal/fdv2/FDv2ChangeSetBuilder'; | ||
| import { DeleteObject, Event, PutObject } from '../../../src/internal/fdv2/proto'; | ||
|
|
||
| it('throws an error when finishing without starting', () => { | ||
| const builder = new FDv2ChangeSetBuilder(); | ||
| expect(() => builder.finish()).toThrow('changeset: cannot complete without a server-intent'); | ||
| }); | ||
|
|
||
| it('starts a new changeset with the given intent', () => { | ||
| const builder = new FDv2ChangeSetBuilder(); | ||
| builder.start('xfer-full'); | ||
| const result = builder.finish(); | ||
|
|
||
| expect(result).toBeDefined(); | ||
| expect(result.length).toBeGreaterThan(0); | ||
| expect(result[0].event).toBe('server-intent'); | ||
| }); | ||
|
|
||
| it('resets events when starting a new changeset', () => { | ||
| const builder = new FDv2ChangeSetBuilder(); | ||
| builder.start('xfer-full'); | ||
| builder.putObject({ kind: 'flag', key: 'test-flag', version: 1, object: {} }); | ||
| builder.start('xfer-full'); | ||
| const result = builder.finish(); | ||
|
|
||
| // Should only have server-intent and payload-transferred, no put-object events | ||
| const putObjectEvents = result.filter((e) => e.event === 'put-object'); | ||
| expect(putObjectEvents.length).toBe(0); | ||
| }); | ||
|
|
||
| it('includes server-intent as the first event with correct structure', () => { | ||
| const builder = new FDv2ChangeSetBuilder(); | ||
| builder.start('xfer-full'); | ||
| const result = builder.finish(); | ||
|
|
||
| const serverIntentEvent = result[0] as Event; | ||
| expect(serverIntentEvent.event).toBe('server-intent'); | ||
| expect(serverIntentEvent.data).toBeDefined(); | ||
|
|
||
| const intentData = serverIntentEvent.data as any; | ||
| expect(intentData.payloads).toBeDefined(); | ||
| expect(intentData.payloads.length).toBe(1); | ||
| expect(intentData.payloads[0].intentCode).toBe('xfer-full'); | ||
| expect(intentData.payloads[0].id).toBe('dummy-id'); | ||
| expect(intentData.payloads[0].target).toBe(1); | ||
| expect(intentData.payloads[0].reason).toBe('payload-missing'); | ||
| }); | ||
|
|
||
| it('includes payload-transferred as the last event with empty state', () => { | ||
| const builder = new FDv2ChangeSetBuilder(); | ||
| builder.start('xfer-full'); | ||
| const result = builder.finish(); | ||
|
|
||
| const payloadTransferredEvent = result[result.length - 1] as Event; | ||
| expect(payloadTransferredEvent.event).toBe('payload-transferred'); | ||
| expect(payloadTransferredEvent.data).toBeDefined(); | ||
|
|
||
| const transferredData = payloadTransferredEvent.data as any; | ||
| expect(transferredData.state).toBe(''); | ||
| expect(transferredData.version).toBe(1); | ||
| expect(transferredData.id).toBe('dummy-id'); | ||
| }); | ||
|
|
||
| it('includes all put and delete events between server-intent and payload-transferred', () => { | ||
| const builder = new FDv2ChangeSetBuilder(); | ||
| const putObj1: PutObject = { | ||
| kind: 'flag', | ||
| key: 'flag-1', | ||
| version: 1, | ||
| object: { key: 'flag-1', on: true }, | ||
| }; | ||
| const deleteObj: DeleteObject = { | ||
| kind: 'segment', | ||
| key: 'segment-1', | ||
| version: 2, | ||
| }; | ||
| const putObj2: PutObject = { | ||
| kind: 'flag', | ||
| key: 'flag-2', | ||
| version: 3, | ||
| object: { key: 'flag-2', on: false }, | ||
| }; | ||
|
|
||
| builder.start('xfer-full'); | ||
| builder.putObject(putObj1); | ||
| builder.deleteObject(deleteObj); | ||
| builder.putObject(putObj2); | ||
| const result = builder.finish(); | ||
|
|
||
| expect(result.length).toBe(5); // server-intent + 3 events + payload-transferred | ||
| expect(result[0].event).toBe('server-intent'); | ||
| expect(result[1].event).toBe('put-object'); | ||
| expect((result[1].data as PutObject).key).toBe('flag-1'); | ||
| expect(result[2].event).toBe('delete-object'); | ||
| expect((result[2].data as DeleteObject).key).toBe('segment-1'); | ||
| expect(result[3].event).toBe('put-object'); | ||
| expect((result[3].data as PutObject).key).toBe('flag-2'); | ||
| expect(result[4].event).toBe('payload-transferred'); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| import { DeleteObject, Event, PutObject } from './proto'; | ||
|
|
||
| // eventually this will be the same as the IntentCode type, but for now we'll use a simpler type | ||
| type supportedIntentCodes = 'xfer-full'; | ||
|
|
||
| /** | ||
| * FDv2ChangeSetBuilder is a helper for constructing a change set for FDv2. | ||
| * The main use case for this builder is to help construct a change set from | ||
| * a FDv1 payload. | ||
| * | ||
| * @experimental | ||
| * This type is not stable, and not subject to any backwards | ||
| * compatibility guarantees or semantic versioning. It is not suitable for production usage. | ||
| */ | ||
| export default class FDv2ChangeSetBuilder { | ||
| private _intent?: supportedIntentCodes; | ||
| private _events: Event[] = []; | ||
|
|
||
| /** | ||
| * Begins a new change set with a given intent. | ||
| */ | ||
| start(intent: supportedIntentCodes): this { | ||
| this._intent = intent; | ||
| this._events = []; | ||
|
|
||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the completed changeset. | ||
| * NOTE: currently, this builder is not designed to continuously build changesets, rather | ||
| * it is designed to construct a single changeset at a time. We can easily expand this by | ||
| * resetting some values in the future. | ||
| */ | ||
| finish(): Array<Event> { | ||
| if (this._intent === undefined) { | ||
| throw new Error('changeset: cannot complete without a server-intent'); | ||
| } | ||
|
|
||
| // NOTE: currently the only use case for this builder is to | ||
| // construct a change set for a file data intializer which only supports | ||
| // FDv1 format. As such, we need to use dummy values to satisfy the FDv2 | ||
| // protocol. | ||
| const events: Array<Event> = [ | ||
| { | ||
| event: 'server-intent', | ||
| data: { | ||
| payloads: [ | ||
| { | ||
| id: 'dummy-id', | ||
| target: 1, | ||
| intentCode: this._intent!, | ||
| reason: 'payload-missing', | ||
| }, | ||
| ], | ||
| }, | ||
| }, | ||
| ...this._events, | ||
| { | ||
| event: 'payload-transferred', | ||
| data: { | ||
| // IMPORTANT: the selector MUST be empty or "live" data synchronizers | ||
| // will not work as it would try to resume from a bogus state. | ||
| state: '', | ||
| version: 1, | ||
| id: 'dummy-id', | ||
| }, | ||
| }, | ||
| ]; | ||
|
|
||
| return events; | ||
| } | ||
|
|
||
| /** | ||
| * Adds a new object to the changeset. | ||
| */ | ||
| putObject(obj: PutObject): this { | ||
| this._events.push({ | ||
| event: 'put-object', | ||
| data: obj, | ||
| }); | ||
|
|
||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Adds a deletion to the changeset. | ||
| */ | ||
| deleteObject(obj: DeleteObject): this { | ||
| this._events.push({ | ||
| event: 'delete-object', | ||
| data: obj, | ||
| }); | ||
|
|
||
| return this; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,29 @@ | ||
| export type EventType = | ||
| | 'server-intent' | ||
| | 'put-object' | ||
| | 'delete-object' | ||
| | 'payload-transferred' | ||
| | 'goodbye' | ||
| | 'error' | ||
| | 'heart-beat'; | ||
| export type IntentCode = 'xfer-full' | 'xfer-changes' | 'none'; | ||
| export type ObjectKind = 'flag' | 'segment'; | ||
|
|
||
| export interface Event { | ||
| event: string; | ||
| data: any; | ||
| event: EventType; | ||
| data: | ||
| | ServerIntentData | ||
| | PutObject | ||
| | DeleteObject | ||
| | PayloadTransferred | ||
| | GoodbyeObject | ||
| | ErrorObject; | ||
|
Contributor
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. If new values are added to the protocol, is the code well behaved? Same for nearby types.
Contributor
Author
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. yea payload changes will potentially break this, but that is probably true even if we didn't declare these types? |
||
| } | ||
|
|
||
| export interface ServerIntentData { | ||
| payloads: PayloadIntent[]; | ||
| } | ||
|
|
||
| export type IntentCode = 'xfer-full' | 'xfer-changes' | 'none'; | ||
|
|
||
| export interface PayloadIntent { | ||
| id: string; | ||
| target: number; | ||
|
|
@@ -17,19 +32,31 @@ export interface PayloadIntent { | |
| } | ||
|
|
||
| export interface PutObject { | ||
| kind: string; | ||
| kind: ObjectKind; | ||
| key: string; | ||
| version: number; | ||
| object: any; | ||
| } | ||
|
|
||
| export interface DeleteObject { | ||
| kind: string; | ||
| kind: ObjectKind; | ||
| key: string; | ||
| version: number; | ||
| } | ||
|
|
||
| export interface GoodbyeObject { | ||
| reason: string; | ||
| silent: boolean; | ||
| catastrophe: boolean; | ||
| } | ||
|
|
||
| export interface ErrorObject { | ||
| payload_id: string; | ||
| reason: string; | ||
| } | ||
|
|
||
| export interface PayloadTransferred { | ||
| state: string; | ||
| version: number; | ||
| id?: string; | ||
| } | ||
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.
Double check logic in the
processFDv1FlagsAndSegmentsinPollingProcessorFDv2matches this and/or consider commonizing.Uh oh!
There was an error while loading. Please reload this page.
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.
ah yea the logic is the same! Let me think of a way to merge the logic.
Now that I am thinking about this more, I don't think
sdk-commonis the right place to create this builder? I am not sure how FDv2 is going to show up for client side implementations so probably move this out tosdk-serverthen, if we find out that client implementation is similar, move it back out tosdk-common? @abarker-launchdarkly @kinyoklion thoughts?Otherwise, I think the plan will be change this to a
FDv1PayloadAdaptorinstead of a builder.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 the data inside the objects is opaque, then the the code itself would likely be the same for both of them.
I am fine with it not being common until we need it though.
A note specifically for internal/common code, but code we intend to use client-side we likely want to avoid classes and do this with functions, as generally speaking they end up smaller when minified. (Or old school function that returns object).