Skip to content
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

Use dequal instead of JSON.stringify for useDeepCallback. #276

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Use dequal instead of JSON.stringify for useDeepCallback. #276

wants to merge 3 commits into from

Conversation

CaveSeal
Copy link

No description provided.

@CaveSeal
Copy link
Author

CaveSeal commented Jun 19, 2020

I'd like to offer this as a solution to #168.

I couldn't see a reason for using deep object comparison on the dependencies for makeFetch, and the JSON.stringify comparison made it so any callback functions specified in the options wouldn't use state in their outer environment. Using the built-in useCallback hook seems to maintain the existing behavior, while also fixing the issue.

However, this might just be my ignorance talking, and you actually need deep comparison, in which case, I would like to know how you feel about using an alternative for deep comparison, such as https://www.npmjs.com/package/dequal, similar to how it's used in https://github.com/kentcdodds/use-deep-compare-effect.

@alex-cory
Copy link
Collaborator

useDeepCallback uses JSON.stringify as suggested here. If useCallback is used, it will cause an infinite loop because of object comparison in the dependencies.

Go ahead and try your last suggestion, but when I did this I looked at the source for use-deep-compare-effect which used dequal. If it works, and all the tests pass, then I will merge the PR. :)

@CaveSeal CaveSeal changed the title Remove useDeepCallback in favor of useCallback Use dequal instead of JSON.stringify for useDeepCallback. Jul 1, 2020
@CaveSeal
Copy link
Author

CaveSeal commented Jul 1, 2020

I think the scenario of infinite looping would only happen if the function references were changing on every render. But, in this case, they seem to be part of the options, and the options are memoized, so I don't think the function references will change unless the options change. That's just my guess.

I've updated the code to use dequal as a dependency, and all seems to behave as expected.

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