-
Notifications
You must be signed in to change notification settings - Fork 59
feat: add debounce hook #1389
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 debounce hook #1389
Conversation
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.
Code Review
This pull request introduces a new debounce hook, which is a great utility. The implementation is mostly solid, but I've found a critical issue in the LRU cache implementation that could lead to incorrect cache eviction. I've also identified a few configuration issues with file paths that might break the build or IDE features. Additionally, there are some minor improvements for documentation, tests, and robustness that I've suggested. Overall, this is a great addition once these issues are addressed.
47789bc to
5aa11ce
Compare
|
/gemini review |
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.
Code Review
This pull request introduces a new debounce hook, which is a great utility for rate-limiting hook executions in environments with frequent re-renders. The implementation is solid, using a fixed-size expiring cache to manage debouncing. I've identified a few areas for improvement, mainly related to configuration paths and documentation clarity, including a couple of high-severity path issues in project.json and tsconfig.base.json that could affect the monorepo tooling. I've also suggested improvements to a test case to make it clearer and more accurate. Overall, this is a well-executed feature.
d073cf7 to
e2586a1
Compare
guidobrei
left a comment
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.
Nice job. Only 2 minor comments.
lukas-reining
left a comment
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.
This looks good, I left some nits you can ignore.
I have question regarding hook data that I would like to clarify before approving :)
|
/gemini review |
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.
Code Review
Thank you for adding the debounce hook. This is a great utility for performance-sensitive applications. I've reviewed the code and found a few issues, including some critical bugs in the caching logic and a few configuration problems. I've also suggested a refactoring to simplify the implementation and improve maintainability. Please take a look at my comments.
|
/gemini review |
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
03697d0 to
477c601
Compare
|
/gemini review |
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.
Code Review
This pull request introduces a new debounce hook, a utility for rate-limiting other hooks, which is a great addition. The implementation is mostly solid, including a custom FixedSizeExpiringCache. However, I've identified a critical issue in the asynchronous handling within the hook that could lead to unhandled promise rejections. Additionally, there are a few configuration and documentation inconsistencies that should be addressed to ensure correctness and maintainability.
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
|
@beeme1mr @lukas-reining ready for re-review... I'm a bit worried about the Gemini comment here though. Either it's broken or I've been working too long. |
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.
This functionally looks really good to me! @toddbaert
But I have problems with the types.
When I do OpenFeatureServer.addHooks in your test I get type mismatch errors:
import { OpenFeature as OpenFeatureServer } from '@openfeature/server-sdk';
........
describe('server-sdk hooks', () => {
const contextKey = 'key';
const contextValue = 'value';
const evaluationContext = { [contextKey]: contextValue };
it('should debounce synchronous hooks', () => {
const innerServerSdkHook: ServerSdkHook = {
before: jest.fn(() => {
return evaluationContext;
}),
after: jest.fn(),
error: jest.fn(),
finally: jest.fn(),
};
const hook = new DebounceHook(innerServerSdkHook, {
debounceTime: 60_000,
maxCacheItems: 100,
});
OpenFeatureServer.addHooks(hook);TS2345: Argument of type DebounceHook<FlagValue> is not assignable to parameter of type Hook<Record<string, unknown>>
The types returned by after(...) are incompatible between these types.
Type
void | EvaluationContext | Promise<void | EvaluationContext>
is not assignable to type void | Promise<void>
Type EvaluationContext is not assignable to type void | Promise<void>
Type EvaluationContext is missing the following properties from type Promise<void>: then, catch, finally, [Symbol.toStringTag]
This only happens for the Server SDKs due to the additional option for the hooks being async. I tried to fix it but ran out of time, the obvious typing changes I had in mind were no success.
I will check again tomorrow.
Co-authored-by: Lukas Reining <lukas.reining@codecentric.de> Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
lukas-reining
left a comment
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.
After our discussions, this looks really good to me!
Co-authored-by: Lukas Reining <lukas.reining@codecentric.de> Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com> Co-authored-by: Michael Beemer <beeme1mr@users.noreply.github.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Lukas Reining <lukas.reining@codecentric.de> Signed-off-by: Thomas Poignant <thomas.poignant@gofeatureflag.org>
Adds "debounce" hook.
This is a utility "meta" hook, which can be used to effectively debounce or rate limit other hooks based on various parameters.
This can be especially useful for certain UI frameworks and SDKs that frequently re-render and re-evaluate flags (React, Angular, etc).
The hook maintains a simple expiring cache with a fixed max size and keeps a record of recent evaluations based on a user-defined key-generation function (keySupplier).
Simply wrap your hook with the debounce hook by passing it a constructor arg, and then configure the remaining options.
In the example below, we wrap a logging hook so that it only logs a maximum of once a minute for each flag key, no matter how many times that flag is evaluated.