-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
feat(UseRefHistoryOptions): add shouldCommit
#4471
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
base: main
Are you sure you want to change the base?
feat(UseRefHistoryOptions): add shouldCommit
#4471
Conversation
someone who understands filters more should confirm im not talking nonsense but i wonder if its better this be implemented as some kind of event filter, such that it never reaches if it turns out that's a silly idea - this PR does look good meanwhile, although i'd be tempted to call it something like edit: basically a filter like this: function equalityFilter<T>(val: Ref<T>, isEqual: (oldValue: T, newValue: T) => boolean): EventFilter {
const lastValue = ref(val.value)
return (invoke) => {
if (!isEqual(lastValue.value, val.value))
lastValue.value = val.value
invoke()
}
}
} but yeah i could just be overcomplicating things 🤷 |
ShouldCommit is a more appropriate name, beforeCommit is more like a hook. |
shouldCommit
Looks good so far. Maybe you can add tests to prevent future regressions. 👍🏽 |
Added :) |
* @param newValue New value | ||
* @returns boolean indicating if commit should proceed | ||
*/ | ||
shouldCommit?: (oldValue: Serialized | undefined, newValue: Raw) => boolean |
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 fine with merging like this, but maybe lets talk quickly about, that it would be nice to have new and old value of the same type.
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.
you mean as shouldCommit?: (oldValue: Raw | undefined, newValue: Raw) => boolean
?
then they would be the same, but is it really better? what do you think speaks in favor of this?
Description
This PR adds a
beforeCommit
option touseRefHistory
that allows users to control whether a commit should proceed based on comparing the previous and new values. This is particularly useful when you want to prevent unnecessary history entries when values haven't meaningfully changed.The
beforeCommit
function receives two parameters:oldValue
: The previous value from historynewValue
: The current value being committedIt returns a boolean indicating whether the commit should proceed.
Example usage:
Additional context
beforeCommit
check is performed just before the actual commitFixes #4468
[x] Read the Contributing Guidelines
[x] Read the Pull Request Guidelines
[x] Checked for duplicate PRs
[x] Provided description of the solution
[x] Added relevant tests (to be added if required by maintainers)