Skip to content

Conversation

@faizanu94
Copy link
Contributor

This pull request introduces value caching and diffing mechanisms to optimize change detection. By caching previous values and diffing them against current values, we ensure that only the property that has changed is passed to the callback function as discussed in #1

Changes

  • Added a value caching system to store previous state values
  • Implemented a diffing algorithm to efficiently compare current values against cached values
  • Modified callback functions to receive only the properties that have changed, based on the diffing results

@bramus
Copy link
Owner

bramus commented Sep 6, 2024

@faizanu94 I’m not sure if you are looking for a review yet or not, but here goes: the core of this PR looks really good.

The only thing that is missing is a flag to get back the old behavior, where an author can choose between getting only these changedProperties (default behavior) or getting back all the monitored properties.

The logic for that is similar to the flag you wrote in #1.

(Once this PR is in, we can pick up #1 again to let users choose between the current simple format, or the more rich format)

@faizanu94 faizanu94 force-pushed the feat/optimized-change-detection branch from b2ad0f1 to bb66704 Compare September 6, 2024 22:27
@faizanu94
Copy link
Contributor Author

Thanks for the reminder about taking care of the old behavior - I’ve added the flag observeAllProperties to toggle between the old and new behaviors, and updated the README accordingly. All set now!

@bramus
Copy link
Owner

bramus commented Sep 9, 2024

I’ve added the flag observeAllProperties to toggle between the old and new behaviors, and updated the README accordingly. All set now!

Instead of a boolean here, this needs to be the { callbackMode: CallbackMode.INDIVIDUAL } that you introduced in #3. Maybe a better name would be NotificationMode.

@faizanu94
Copy link
Contributor Author

Thanks for the feedback! I've replaced the boolean flag with NotificationMode as suggested, using NotificationMode.INDIVIDUAL and NotificationMode.ALL. This approach can easily scale to other options like ReturnFormat in the future. README has been updated accordingly

Copy link
Owner

@bramus bramus left a comment

Choose a reason for hiding this comment

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

This is looking great! In hindsight the INDIVIDUAL mode is incorrectly named, as it can report two changed properties at once.

I have made suggestions (which you can apply in the GitHub UI by simply clicking the button) to rename it to CHANGED_ONLY.

Also reworded the text in the README.

faizanu94 and others added 4 commits September 15, 2024 01:19
Co-authored-by: Bramus <bramus@bram.us>
Co-authored-by: Bramus <bramus@bram.us>
Co-authored-by: Bramus <bramus@bram.us>
Co-authored-by: Bramus <bramus@bram.us>
@faizanu94 faizanu94 requested a review from bramus September 14, 2024 21:20
@faizanu94
Copy link
Contributor Author

Thanks for the suggestions - I’ve applied the changes. Appreciate the guidance!

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