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

feat: Add typed variation methods. #288

Merged

Conversation

kinyoklion
Copy link
Member

@kinyoklion kinyoklion commented Sep 27, 2023

This adds typed variation methods.

This works for both JS and TS, but may be especially nice for those that want to be more type safe in typescript.

resolves #285

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #218820: Add typed variation methods to node server v9..

@@ -31,7 +31,8 @@ app.get('/', (req, res) => {
'migrations',
'event-sampling',
'config-override-kind',
'metric-kind'
'metric-kind',
'strongly-typed',
Copy link
Member Author

Choose a reason for hiding this comment

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

Run the typed contract tests.

switch(pe.valueType) {
case "bool":
return await client.boolVariationDetail(pe.flagKey, pe.context || pe.user, pe.defaultValue);
case "int":
Copy link
Member Author

Choose a reason for hiding this comment

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

The contract tests support int and double, but we are using a single numberVariation.

@@ -160,6 +160,99 @@ describe('given an LDClient with test data', () => {
const valueB = await client.variation('my-feature-flag-1', userContextObject, 'default');
expect(valueB).toEqual(true);
});

it('evaluates with jsonVariation', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

This just addresses the problem with any in Typescript. Some people require using unknown so they have to explicitly cast.

const stringRes: string = (await client.jsonVariation('flagkey', defaultUser, false)) as string;
expect(stringRes).toBe('potato');
});

Copy link
Member Author

Choose a reason for hiding this comment

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

Below are all the typed method calls.

@@ -57,7 +57,7 @@ describe('given an LDClient with test data', () => {
const defaultValue = Object.values(LDMigrationStage).find((item) => item !== value);
// Verify the pre-condition that the default value is not the value under test.
expect(defaultValue).not.toEqual(value);
const res = await client.variationMigration(
const res = await client.migrationVariation(
Copy link
Member Author

Choose a reason for hiding this comment

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

I decided to make this the same as the other SDKs with this change. This has not been released yet (for Yus).

@kinyoklion kinyoklion marked this pull request as draft September 27, 2023 22:15
}

async boolVariation(key: string, context: LDContext, defaultValue: boolean): Promise<boolean> {
return (
Copy link
Member Author

Choose a reason for hiding this comment

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

The approach taken with typedEval is to have it take a validator that checks the type and also indicates what the type was, for error messages. This matched nicely with the TypeValidators implementation.

If the type is wrong, then it generates the correct reason and evaluation events.

@kinyoklion kinyoklion marked this pull request as ready for review September 27, 2023 22:19
@kinyoklion kinyoklion merged commit 8e96a52 into feat/node-migrations Sep 27, 2023
13 checks passed
@kinyoklion kinyoklion deleted the rlamb/sc-218820/typed-variation-methods branch September 27, 2023 22:36
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