Skip to content

Comments

Give users more flexibility in writing handleSet fn (alternative solution to PR #141)#142

Closed
pstrassmann wants to merge 3 commits intocharkour:mainfrom
pstrassmann:pstrassmann-handleSet-changes-option2
Closed

Give users more flexibility in writing handleSet fn (alternative solution to PR #141)#142
pstrassmann wants to merge 3 commits intocharkour:mainfrom
pstrassmann:pstrassmann-handleSet-changes-option2

Conversation

@pstrassmann
Copy link
Collaborator

@pstrassmann pstrassmann commented Dec 16, 2023

This is an alternative solution to addressing the issues fixed in #141

In this solution, rather than changing the conditions under which handleSet is called like in the other PR, users instead can write a handleSet fn that returns a fn that, in addition to receiving pastState as before, now also receives currentState.

This allows users to return a function that can, based on their own logic around pastState and currentState, determine when to run handleSet.

This allows users to write a handleSet function like:

 handleSet: (handleSet) => {
          const throttleFn = throttle((state) => {
              handleSet(state);
            }, 1000);

         // return a function that will run a throttle fn that runs handleSet, but only if state has changed according to
         // user defined logic
          return (
            pastState: Partial<MyState>,
            currentState?: Partial<MyState>,
          ) => {
            const hasStateChanged =
              diff(pastState, currentState ?? {}).length > 0;
            if (hasStateChanged) {
              throttleFn(pastState);
            }
          };
        },

while remaining compatible and non-breaking with existing handleSet functions written like:

 handleSet: (handleSet) => {
          return throttle((pastState) => {
            handleSet(pastState);
          }, 1000);
        },

This solution has been tested locally, and 1 relevant test has been added demonstrating a handleSet fn that uses currentState to conditionally run the throttle fn that runs handleSet

@charkour
Copy link
Owner

charkour commented Dec 18, 2023

Thanks you for making this PR, adding tests, and providing a thorough description!

I'll have some time later today to review this in depth and provide some feedback. Thank you!

@charkour charkour self-requested a review December 18, 2023 17:13
@charkour
Copy link
Owner

charkour commented Dec 18, 2023

This "leave it to the user" approach is awesome! I think I prefer this to the other PR proposal, but I'm wondering if the bug is solvable by tweaking the options in the original issue you raised. I'll keep playing around with it, but my gut says we'll likely go with this solution here.

@pstrassmann pstrassmann mentioned this pull request Dec 18, 2023
Closed
@pstrassmann pstrassmann force-pushed the pstrassmann-handleSet-changes-option2 branch from 820eeb5 to e03ab0c Compare December 23, 2023 16:50
@pstrassmann
Copy link
Collaborator Author

pstrassmann commented Dec 23, 2023

As an update on this -- I've been able to successfully use a patch based on this PR for our application to resolve the issue of throttles getting triggered by untrackedState. I still think something like this makes a lot of sense to bring in to the zundo though!

I realized I did not update the README in this PR, so I will do that very soon. There is at least one interesting catch to describe, like the fact that if one uses an equality fn within handleSet ZundoOption to conditionally call handleSet, one does not also need to use an equality fn ZundoOption (unless you want the equality fns to be different).

@charkour
Copy link
Owner

charkour commented Dec 23, 2023 •
edited
Loading

Thanks for the additional update on this! I've been thinking through some possible work around/ideal solutions that don't require a core change that I'd like to run past you before we decide to go forward with this PR.

Glad you have a patch! My thought process is 1) the workarounds an are sufficient for your use case at work or 2) the workarounds aren't sufficient and we move forward with this PR. Either way, the goal will be to remove the patch from your code. It's just a temporary hold over while we make a decision. Thanks for your patience on this!

@pstrassmann
Copy link
Collaborator Author

pstrassmann commented Dec 23, 2023

Ok yeah that sounds good! I am definitely open to exploring and chatting about other solutions or work arounds. Thank you!

@charkour charkour self-assigned this Dec 23, 2023
@charkour
Copy link
Owner

charkour commented Dec 23, 2023

Hi @pstrassmann, after taking time to verify my workarounds didn't work, I spent time reviewing the PRs. Both are great, but I think the best is a combination of the two. #141 is closer to what I imagine, but we can pass more data to the curriedHandleSet function.

What do you think? If you agree, let's close this PR and you can make updates to #141

@pstrassmann pstrassmann mentioned this pull request Dec 24, 2023
Merged
@charkour
Copy link
Owner

charkour commented Jan 14, 2024

Closing in favor of #149

@charkour charkour closed this Jan 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@charkour charkour Awaiting requested review from charkour

Assignees

@charkour charkour

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@pstrassmann @charkour @pstrassmann-gfm