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

💄 Fix memoization issues #266

Merged
merged 4 commits into from
Mar 23, 2022
Merged

💄 Fix memoization issues #266

merged 4 commits into from
Mar 23, 2022

Conversation

wcandillon
Copy link
Contributor

based on @nandorojo comments

@wcandillon wcandillon requested a review from chrfalch March 17, 2022 08:31
@@ -66,7 +65,7 @@ export const useSharedValueEffect = <T = number>(
"worklet";
runOnJS(cb)();
},
triggers,
[value, ...values],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chrfalch no strong opinon here, we could keep the trigger variable

@wcandillon wcandillon changed the title Fix memoization issues 💄 Fix memoization issues Mar 17, 2022
@chrfalch chrfalch merged commit c85944b into main Mar 23, 2022
@chrfalch chrfalch deleted the fix/memo branch March 23, 2022 14:13
@nandorojo
Copy link

nandorojo commented Mar 23, 2022

One small thing I'm now noticing: what if useDerivedValue had the same API as reanimated's, where you don't need to read in the args from the factory function?

const dv = useDerivedValue(() => value, [value])

This works with the code in this PR.

This way it's more like other react hooks. I find passing the values to an array and then also to the args a bit awkward.

Technically, users could do the above with the current implementation. It's just a matter of how it's documented. If you went with that API, it would help to document that users add useDerivedValue to their react hooks ESLint settings.

@wcandillon
Copy link
Contributor Author

thanks for the feedback, it is fixed now.
Passing the values as args had some issue and it made it easy to shot yourself in the foot (if you change the order of the array for instance)

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.

3 participants