-
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 migration configuration and basic migration. #213
feat: Add migration configuration and basic migration. #213
Conversation
/** | ||
* Configuration class for configuring serial execution of a migration. | ||
*/ | ||
export class LDSerialExecution { |
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 am not 100% on these being classes. But it can be easily changed if it proves cumbersome to use from JS.
fromNew: LDMethodResult<TMigrationRead>; | ||
}; | ||
|
||
async function safeCall<TResult>( |
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 don't trust people to not have exceptions, so this handles an exception and turns it into an error return.
* | ||
* An implementation may also throw an exception to represent an error. | ||
*/ | ||
export type LDMethodResult<TResult> = |
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've gone with a rust-like tagged union.
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'm not going to add monadic operation.)
}); | ||
|
||
describe.each([ | ||
[new LDSerialExecution(LDExecutionOrdering.Fixed), 'serial fixed'], |
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 don't have builders, but these "execution" classes ensure correct combinations without having a potentially unused config at the top level.
])( | ||
'uses the correct authoritative source: %p, read: %p, write: %p.', | ||
async (stage, readValue, writeResult) => { | ||
const migration = new Migration<string, boolean>(client, { |
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.
The location of the Migration type may not be permanent yet, and I need to decide if you directly construct one, or if we have a factory. The SDK has not exposed any implementation types before.
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.
These changes look good. One point for consideration, but nothing that couldn't be addressed in a follow-up.
* Results from `readOld` or `writeOld` will be 'old'. | ||
* Results from `readNew` or `writeNew` will be 'new'. | ||
*/ | ||
export type LDMigrationOrigin = 'old' | 'new'; |
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 the spec, I was thinking it might be useful to designate whether they were old or new sources, as well as if they were authoritative or not. This way the caller doesn't have to encode our same mapping table, and we can more easily change it in the future. What do you think?
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.
Yeah, I see how, at least for writes, that information could provide some value. I will try it.
This does not have consistency tracking, error tracking, or latency tracking.
Most documentation is temporary.