Skip to content

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

JonathanSchndr
Copy link

@JonathanSchndr JonathanSchndr commented Jan 7, 2025

Description

This PR adds a beforeCommit option to useRefHistory 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 history
  • newValue: The current value being committed

It returns a boolean indicating whether the commit should proceed.

Example usage:

const { history } = useRefHistory(myRef, {
  beforeCommit: (oldValue, newValue) => {
    return JSON.stringify(oldValue) !== JSON.stringify(newValue)
  }
})

Additional context

  • The implementation preserves all existing functionality
  • The beforeCommit check is performed just before the actual commit
  • All original comments and documentation have been maintained
  • The change is fully typed with TypeScript
  • This feature is particularly useful for array/object references where you want to compare actual content rather than just references

Fixes #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)

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. enhancement New feature or request labels Jan 7, 2025
@JonathanSchndr JonathanSchndr changed the title feat(UseRefHistoryOptions): add beforeCommit #4468 feat(UseRefHistoryOptions): add beforeCommit Jan 7, 2025
@43081j
Copy link
Collaborator

43081j commented Jan 8, 2025

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 commit (similar to how you can probably do that now via the eventFilter option)

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 shouldCommit since its specifically to check if it should commit, rather than being a plain hook

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 🤷

@tetap
Copy link

tetap commented Jan 10, 2025

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 commit (similar to how you can probably do that now via the eventFilter option)

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 shouldCommit since its specifically to check if it should commit, rather than being a plain hook

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.
eventFilter currently does not support multiple scenarios. If equalityFilter is used, debounceFilter will fail again. This should be a broader scenario and a heavier workload. I think this design is the best. Just consider whether you really need to add multiple to eventFilter.

@OrbisK OrbisK changed the title feat(UseRefHistoryOptions): add beforeCommit feat(UseRefHistoryOptions): add shouldCommit Jan 19, 2025
@OrbisK
Copy link
Collaborator

OrbisK commented Jan 19, 2025

Looks good so far. Maybe you can add tests to prevent future regressions. 👍🏽

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Feb 3, 2025
@JonathanSchndr
Copy link
Author

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
Copy link
Collaborator

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.

Copy link
Author

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?

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Apr 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
4 participants