-
Notifications
You must be signed in to change notification settings - Fork 18
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 migrationVariation method. #212
feat: Add migrationVariation method. #212
Conversation
@@ -24,7 +24,7 @@ | |||
"build": "npx tsc", | |||
"clean": "npx tsc --build --clean", | |||
"lint": "npx eslint . --ext .ts", | |||
"lint:fix": "yarn run lint -- --fix" | |||
"lint:fix": "yarn run lint --fix" |
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.
Unrelated minor fix to run lint.
if (stringValue && !IsMigrationStage(stringValue)) { | ||
const error = new Error(`Unrecognized MigrationState for "${key}"; returning default value.`); | ||
this.onError(error); | ||
callback?.(error, defaultValue); |
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.
Most of the implementation here is just to handle the compatibility we have with callbacks. Everything supports both a callback and a promise. The error handling is also a little unique, we raise error events or put errors in callbacks.
async variationMigration( | ||
key: string, | ||
context: LDContext, | ||
defaultValue: LDMigrationStage, |
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.
As long as they do not break the typing the default value is of type LDMigrationStage, so I am not validating the default value.
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.
Though if it was not the correct type, and it came back out as the default, it would still get validated.
if (!IsMigrationStage(stringValue)) { | ||
const error = new Error(`Unrecognized MigrationState for "${key}"; returning default value.`); | ||
this.onError(error); | ||
callback?.(error, defaultValue); |
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.
I may consider removing callback support entirely for this method, but I will do that in another change after some discussion.
For now I am adding this to server common, which means that edge SDKs would inherit the behavior. We may not want that, because they don't support events, but I can always exclude it or move the implementation.