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: correct behavior of function setState parameter #598

Merged
merged 1 commit into from
Nov 7, 2023

Conversation

swain
Copy link
Contributor

@swain swain commented Nov 7, 2023

Fixes #599.

Please read the linked issue for a detailed description of the issue being fixed here.

Changes

This change is a one-liner, nice and simple.

I had hoped to write a test to validate the change + prevent future regressions, but I didn't see a test suite.
I didn't want to make this PR too big, so I opted to add a test suite and a relevant test in a separate PR: #600.

@swain swain changed the title fix: correct hook behavior by using useSyncExternalStore fix: correct behavior of function setState parameter Nov 7, 2023
@swain swain marked this pull request as ready for review November 7, 2023 08:08
@mrousavy
Copy link
Owner

mrousavy commented Nov 7, 2023

Thanks for your PR! I read the issue and your diff and yep, it definitely was a bug before.

The reason valueRef existed was for performance, as calling getter(..) is a C++ JSI call, which is much more expensive than just getting a simple JS value (here, a Ref).

This fixes the bug, so let's merge this for now, and I don't think that this performance squeeze is really required here, we're not setting state that often that it really makes a difference.

Thanks man, LGTM!

@mrousavy mrousavy merged commit f391f9b into mrousavy:master Nov 7, 2023
2 of 3 checks passed
@swain swain deleted the use-sync-external-store branch November 7, 2023 13:59
@swain
Copy link
Contributor Author

swain commented Nov 7, 2023

@mrousavy Thanks for that additional context, and for the quick review!

Q: do I need to do anything else to get this fix published to NPM? Or, will that happen automatically?

@mrousavy
Copy link
Owner

mrousavy commented Nov 9, 2023

Will do it soon! Thanks

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.

"setState" from useMMKV<Type> hooks does not behave 100% correctly with function parameter
2 participants