Skip to content
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

chore: implement variation functions #298

Conversation

yusinto
Copy link
Contributor

@yusinto yusinto commented Oct 12, 2023

  • Share EventFactory and friends
  • Implement variation, typedVariation, allFlags and other functions for sdk-client

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #220491: Move evaluation classes to shared/common.

@yusinto yusinto marked this pull request as draft October 12, 2023 21:12
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mostly ripped from the previous EventFactory implementation. All functions are identical as previously, except evalEvent. Each sdk-server and sdk-client implements their own copy of evalEvent, and then calling this base function. There maybe a better way to achieve this in typescript, and I'm open for suggestions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just rudimentary tests so not complete coverage yet. I'll add more in future prs.

Comment on lines 89 to 93
const result: LDFlagSet = {};
Object.entries(this.rawFlags).forEach(([k, r]) => {
result[k] = r.value;
});
return result;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is different from the existing js-sdk allFlags. Here I am following the server sdk to not send events on allFlags, thus getting rid of sendEventsOnlyForVariation. I don't understand the use case for sending events in allFlags, and I am pushing for a consistent behavior with the server sdk.

close(onDone?: () => void): Promise<void> {
return Promise.resolve(undefined);
close(): void {
this.eventProcessor.close();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Breaking api change here, again I am pushing for consistency with the server sdk by getting rid of the callback arg and returning void instead of a promise.

Comment on lines 100 to 107
async flush(callback?: (err: Error | null, res: boolean) => void): Promise<void> {
try {
await this.eventProcessor.flush();
} catch (err) {
callback?.(err as Error, false);
}
callback?.(null, true);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a duplicate of sdk-server. We may be able to refactor this plus other similar functions to an LDClientImplBase class (a mouthful).

Comment on lines 134 to 144
track(key: string, data?: any, metricValue?: number): void {
const checkedContext = Context.fromLDContext(this.context);
if (!checkedContext.valid) {
this.logger?.warn(ClientMessages.missingContextKeyNoEvent);
return;
}

this.eventProcessor.sendEvent(
this.eventFactoryDefault.customEvent(key, checkedContext!, data, metricValue),
);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another function which is almost a carbon copy of sdk-server, differing only in the LDContext argument.

);
}

private sendEvalEvent(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And another

);
}

private variationInternal(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic here is copied from sdk-server, however this version is synchronous so does not have callbacks like the server counterpart. Still a lot of opportunities for dry, but I think this is good enough for now.

@@ -1,16 +1,14 @@
import { internal, LDEvaluationDetail, LDEvaluationReason } from '@launchdarkly/js-sdk-common';

import { LDEvaluationDetail, LDEvaluationReason } from '../../api';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a little hesitant on this file. Server SDKs do an eval, so they always produce details and reasons, but client SDKs either have reasons or do not have reasons based on how they were initialized and maybe some other factors. So, for typing I would prefer they remain required in server, but tney are going to need to be optional for clients.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I understand your concerns. I re-visited sdk-client's variationInternal and I think I can accommodate your request. I'll put refactor and submit a secondary PR. This should address your other comment re Reasons as well.

}

flush(onDone?: () => void): Promise<void> {
return Promise.resolve(undefined);
async flush(callback?: (err: Error | null, res: boolean) => void): Promise<void> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to keep the callback, or can we just use a promise instead?

}

getContext(): LDContext {
return { kind: 'user', key: 'test-context-1' };
return { ...this.context };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is problematic. Destructing here only decouples the top level entries. The nested items within the context would still be shared with the original.

It looks like the old implementation did a proper clone (js-sdk-common).

You could clone using cloneWithRedactions and passing an empty private attribute list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok good catch object spread only does first level clone. I'll fix this.

@yusinto yusinto marked this pull request as ready for review October 12, 2023 23:06
Comment on lines -32 to 34
detail: LDEvaluationDetail,
version?: number,
variation?: number,
Copy link
Contributor Author

@yusinto yusinto Oct 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed detail because we are only using it for variationIndex and value. We already have variation on line 34 so it's redundant to have both variationIndex and variation. I think it's safe to replace detail with just value and then use variation as is.

}

getContext(): LDContext {
return { kind: 'user', key: 'test-context-1' };
return clone(this.context);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied the legacy function from js-common-sdk because cloneWithRedactions takes LDContextCommon and not LDContext.

@@ -0,0 +1,3 @@
export default function clone(obj: any) {
return JSON.parse(JSON.stringify(obj));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this will be fine for now, but it does contain some weaknesses compared to how we do it for filtering.

Primarily this will throw an exception for circular contents and some other situations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I understand. in the interest of time i will defer improvements to this to a later date.

@kinyoklion kinyoklion merged commit fc2c212 into feat/merge-client-and-migrations Oct 13, 2023
14 checks passed
@kinyoklion kinyoklion deleted the yus/sc-220491/move-evaluation-classes-to-shared-common branch October 13, 2023 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants