-
-
Notifications
You must be signed in to change notification settings - Fork 189
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
Reference to a computed property seems to trigger unnecessary re-renders #732
Comments
@ROTGP, yes, I agree something seems off here. I recently started carefully analyzing some components for performance and noticed the same issue and was coming to report it when I saw you beat me to it. @ctrlplusb to expand on the example provided by the OP, also see this sandbox: https://codesandbox.io/s/serene-leavitt-fg9p5l I've added an additional computed property and a hook which tracks and logs when a component's deps change. The two computed props are: completedTodos: computed((state) => state.todos.filter((todo) => todo.done)),
safeCompletedTodos: computed([(state) => state.todos], (todos) =>
todos.filter((todo) => todo.done)
), According to the docs it should not be necessary to use state resolvers unless we want to access state from a different slice of our model:
In the sandbox example, In other words, it seems like any state change is causing the computed property to be re-evaluated, rather than just the pieces of state the prop depends on. @ROTGP can you by any change pinpoint a previous >= 5.0.0 version where this was working for you? In the meantime, you can try isolating the pieces of state you depend on using resolvers which seems to work for me. EDIT: I've tested this with all >= 5.0.0 versions in the code sandbox and it seems to be a problem in all versions. Unless this isn't actually a problem and we're supposed to be using state resolvers to isolate every piece of state? In which case maybe just the docs need to be updated? |
@no-stack-dub-sack I'm new to easy-peasy (and React in general), I was just evaluating it and noticed the inconsistent behaviour. I can't say whether it ever worked as described. Thanks for pinpointing the problem and providing the workaround |
@ctrlplusb Any chance you can comment on this and let us know if this is working as expected or if this seems like a bug? Thanks! |
@ctrlplusb gentle nudge on this ☝️ |
I'm not able to reproduce this issue in the given sandbox. Am I missing something? Some general gotchas to be aware of:
const { todos } = useStoreState((state) => state); // 👈 will change whenever the root `state` changes
// vs
const todos = useStoreState((state) => state.todos); // 👈 will only change whenever `todos` changes
|
@jmyrland I've created another code sandbox which perhaps better illustrates the problem / question: https://codesandbox.io/s/friendly-noyce-hoxzq7. Note that While So the question here is: are state resolvers mandatory to prevent components that reference computed props from re-rendering unnecessarily any time store state changes? The docs seem to suggest that state resolvers are not required for simple use-cases, and that internal memoization should adequately handle this. However this example seems to contradict that. The issue that I ran into as a result, was that for complex computed props that relied on many individual pieces of state, I ran into type issues since the e.g. export const someComputedProp: Computed<Model, ReturnType> = computed(
[
(state) => state.foo,
(state) => state.bar,
(state) => state.baz,
(state) => state.bam,
],
(
foo,
bar,
baz,
bam,
) => {
// compute something
},
); vs. export const someComputedProp: Computed<Model, ReturnType> = computed(
[
(state) => ({
foo: state.foo,
bar: state.bar,
baz: state.baz,
bam: state.bam
}) // is this ok?
],
({
foo,
bar,
baz,
bam,
}) => {
// compute something
},
); |
Based on your example, it seems that state resolvers seems to fix the issue. I'm not sure about why, but I'm guessing when not providing state resolvers, the computed props are always regenerated once the store (parent of the computed prop) changes. Once you provide state resolvers, I think it's easier for easy-peasy to figure out when to regenerate the computed prop (only when the state resolvers change). Again, I'm not familiar with the details.
I'm not sure if this works - but if it does, you would effectively be updating the computed prop whenever As for the limit of resolvers, I think this is strictly a typescript limitation - because the state resolvers generates the argument types for the compute function. I think there should be a way to circumvent this (forcing |
An attempt to optimize computed props, by improving comparison of inputs & by checking if the value needs an update. The value comparison uses a naive comparison of of the JSON stringified values. This will support more complex inputs & avoid unneccessary updates for complex results (arrays, objects). related to ctrlplusb#732
@jmyrland Thanks, I haven't tried that but I think you're right that it would be the same as not using resolvers since the returned object would depend on the entire state object. As far as resolvers, this is just a "types" issue, and not a typescript issue. The current Anyway, I appreciate your reply / you looking into this - did your changes have any impact on the issue? |
@jmyrland @ROTGP I've dug into the code a bit more and found out that unless you use state resolvers, the input to the computed property is the parent store's state. Knowing this, it makes perfect sense that the component that references the computed prop re-renders any time that any state changes (not just the piece of state it references). I think then, it's just a misunderstanding of this from the docs:
This does call out that there are perf benefits by isolating local state, I guess it's just what is considered "insignificant" that is open to interpretation here. It would depend on how heavy the component is that's re-rendering due to referencing a computed prop, and how often state in the parent store is likely to change. IMHO, unless there are adverse performance implications to using state resolvers in basic use-cases, then it would seem prudent to default to using them always in order to avoid un-wanted re-renders of the components that consume them. It seems like this is not an actual bug and is in-fact intended behavior, but perhaps the docs could be a little clearer on this. Also, to support more complex computed props, the types could be expanded to allow a greater number of resolvers. I'd be happy to open a PR for this if @ctrlplusb were interested. Thanks again for helping to look into it! |
Haven't tested it, but I think the PR above should solve the issue. It does a "better" job comparing complex types (input from resolvers & the computed result), instead direct comparison. This should:
It's currently just using |
An attempt to optimize computed props, by improving comparison of inputs & by checking if the value needs an update. The value comparison uses a naive comparison of of the JSON stringified values. This will support more complex inputs & avoid unneccessary updates for complex results (arrays, objects). related to ctrlplusb#732
* Optimize computed props An attempt to optimize computed props, by improving comparison of inputs & by checking if the value needs an update. The value comparison uses a naive comparison of of the JSON stringified values. This will support more complex inputs & avoid unneccessary updates for complex results (arrays, objects). related to #732 * refactor: Modifies computed properties to use fast-deep-equals * fix: Computed properties optimisations Co-authored-by: Sean Matheson <sean@ctrlplusb.com>
#764 should solve this issue: POC using easy-peasy@5.1.0-alpha.2 |
Just released Thanks for championing this fix @jmyrland And thanks for all the patience ya'll! |
Thank you @jmyrland and @ctrlplusb! This is awesome! |
* Optimize computed props An attempt to optimize computed props, by improving comparison of inputs & by checking if the value needs an update. The value comparison uses a naive comparison of of the JSON stringified values. This will support more complex inputs & avoid unneccessary updates for complex results (arrays, objects). related to #732 * refactor: Modifies computed properties to use fast-deep-equals * fix: Computed properties optimisations Co-authored-by: Sean Matheson <sean@ctrlplusb.com>
* Optimize computed props An attempt to optimize computed props, by improving comparison of inputs & by checking if the value needs an update. The value comparison uses a naive comparison of of the JSON stringified values. This will support more complex inputs & avoid unneccessary updates for complex results (arrays, objects). related to #732 * refactor: Modifies computed properties to use fast-deep-equals * fix: Computed properties optimisations Co-authored-by: Sean Matheson <sean@ctrlplusb.com>
Using
v5.0.4
, I've noticed that making a reference to computed store properties causes the component making the reference to re-render, even when unrelated properties are being updated. I've made a demo to illustrate:https://codesandbox.io/s/interesting-morning-0qbd4u
Open the console and notice the following output (which is correct):
Now click on either of the two
Increment counter
buttons (one in theBar
component, and the other in theBaz
component), which will increment the store'scounter
property. Note that theFoo
component is the only component to make reference tocounter
, and as such you'd only expectrendering FOO
to be output. However, you'll notice the following in the console output:Commenting out the reference to
completedTodos
(a computed property) in theBar
component will result in it not being mistakenly re-rendered whencounter
is updated.Baz
component has a reference totodos
(a standard, non-computed property) which does not result in a re-render whencounter
is updated.In summary, having a reference to a computed property is causing the referring component to re-render, when I believe it shouldn't. Making a similar reference to a non-computed property does NOT cause the re-render (even when the computed property refers only to the non-computed property). Is this expected behaviour? Is there something I'm missing?
The text was updated successfully, but these errors were encountered: