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: computed props w/ dependencies on store state #874

Merged
merged 1 commit into from
Oct 13, 2023

Conversation

no-stack-dub-sack
Copy link
Collaborator

@no-stack-dub-sack no-stack-dub-sack commented Oct 12, 2023

  • Changes storeState === prevStoreState || areInputsEqual(...) to storeState === prevStoreState && areInputsEqual(...) when considering whether or not to return the cached value of a computed prop. Only if both conditions are true should we blindly return the cached value. If either condition is false, we should re-calculate and find out if the value has changed. If it has not, return the cached value still (this may happen if inputs are equal and store state has changed that the computed prop does not depend on). Otherwise update cached value and trigger re-render.
  • I believe this is related to the changes introduced in Optimize computed props #764. This PR optimized computed props to reduce/remove the number of unwanted re-renders, however the optimization was maybe a bit too aggressive and missed a few edge cases where it doesn't trigger a re-render when it should.
  • The new test fails prior to this code change and illustrates the issue that prompted the filing of Computed props which depend on computed props that consume store state don't update as expected #873.
  • This PR also removes an un-used argument from the createComputedPropertyBinder and changes the runOnce variable name to hasRunOnce (just for a little more clarity - this always confused be a bit at first).

fixes #873

@vercel
Copy link

vercel bot commented Oct 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
easy-peasy ❌ Failed (Inspect) Oct 12, 2023 2:16pm

Copy link
Collaborator

@jmyrland jmyrland left a comment

Choose a reason for hiding this comment

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

LGTM 👌

@no-stack-dub-sack no-stack-dub-sack merged commit 282fa1c into master Oct 13, 2023
7 of 8 checks passed
@no-stack-dub-sack no-stack-dub-sack deleted the fix/dependent-computed-props branch October 13, 2023 16:00
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.

Computed props which depend on computed props that consume store state don't update as expected
2 participants