-
Notifications
You must be signed in to change notification settings - Fork 21
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: Adds LDDataSystemOptions for configuring the Data System. #794
base: ta/sdk-857/composite-datasource
Are you sure you want to change the base?
chore: Adds LDDataSystemOptions for configuring the Data System. #794
Conversation
updateProcessorFactory: TypeValidators.Function.is(validatedOptions.updateProcessor) | ||
? validatedOptions.updateProcessor | ||
: () => validatedOptions.updateProcessor, | ||
}; |
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.
For reviewers: Not sure if we need to continue maintaining these ts-ignore annotations due to the types of the inputs being "object | function". I think we do need to maintain them, perhaps you know off the top of your head @kinyoklion
initSuccessHandler: VoidFunction, | ||
errorHandler?: (e: Error) => void, | ||
) => subsystem.LDStreamProcessor; | ||
} |
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.
Investigate if these should be readonly.
isStreamingOptions(config.dataSystem.dataSource)) && | ||
config.dataSystem.dataSource.streamInitialReconnectDelay | ||
? { reconnectTimeMillis: config.dataSystem.dataSource.streamInitialReconnectDelay } | ||
: null), |
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.
For reviewers: This part got a little ugly since now some of these options may not be present. Is there a cleaner way to omit these members?
this.featureStoreFactory = () => validatedOptions.featureStore; | ||
} | ||
|
||
this.hooks = validatedOptions.hooks; |
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.
For reviewers: Hooks was moved up. UpdateProcessorFactory and FeatureStoreFactory was moved up to be near the data system logic.
e669f87
to
d186358
Compare
d186358
to
bb47dde
Compare
Requirements
I have added test coverage for new or changed functionality
I have followed the repository's pull request submission guidelines
I have validated my changes against all supported platform versions
Will be done on target temp branch eventually.
Related issues
SDK-857 and SDK-1073