Optimize computed props

[jmyrland]

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

[jmyrland]

What do you think about this @ctrlplusb ? I haven't testet this against our codebase, but it should work in theory - and should avoid some unnecessary re-renders for computed props.

[ctrlplusb]

Hey @jmyrland!

Thanks for jumping in with this PR. Interesting approach. I'd love to test this out in an alpha and see how it performs against some real world projects.

I'll try have a little play. :)

[jmyrland]

Hey @jmyrland!

Thanks for jumping in with this PR. Interesting approach. I'd love to test this out in an alpha and see how it performs against some real world projects.

I'll try have a little play. :)

We did some testing of which "compare"-method was fastest, to minimize performance hits. So far it seems that the lodash.isEqual-implementation is by far the fastest. But the JSON.stringify-version should work fine as a POC.

If you could release an alpha, I can test it in all our apps to see if there is any breaking changes. There is also this sandbox that can verify the change.

I'm also betting that @ROTGP and @no-stack-dub-sack is eager to test out this :)

[ctrlplusb]

Sure thing.

So in your investigation we had to perform a deep equality comparison?

I've used this library at a previous client which is pretty blazing;

https://www.npmjs.com/package/fast-deep-equal

[ctrlplusb]

Aha, I think I just made a good discovery.

When doing comparison the computed property being checked is included in the comparison. This should not be the case. It causes stack overflow issues and could be the root cause for some of the strange edge cases we are seeing around computed properties.

Got some interesting ideas how to approach this, and hopefully have minimal impact on perf too.

[ctrlplusb]

@jmyrland

Can you please test out easy-peasy@5.1.0-alpha.1

👍

[jmyrland]

Sure thing.

So in your investigation we had to perform a deep equality comparison?

I've used this library at a previous client which is pretty blazing;

https://www.npmjs.com/package/fast-deep-equal

My theory is that computed arrays & objects are always updated, even if the value is the same - because of the reference comparison (objects & arrays will always fail due to the === comparison).

My attempt to resolve this, was to compare the new & previous computed result before updating the value. In my head, this should avoid unnecessary re-renders for computed arrays & objects.

I'll try out the alpha for the sandbox & report back 👍

[jmyrland]

Dang it - it doesn't work for the sandbox.

I was expecting "Bar render" to be 0.

Question: what default state resolvers are used (if not passing in any) to the computed prop?

I'm guessing it is the parent store for the computed prop? If so, is the inputs are different - but the result should be the same, thus there should not be an update for completedTodos.

[jmyrland]

It is working - I was just referencing the incorrect alpha version 😁 I updated the sandbox with the correct version.

Nice one 🎉🎉

[jmyrland]

We're using a proxied package manager, and I'm not able to find the updated alpha-version. Unable to test it "for real" atm.

[ctrlplusb]

Awesome!!

This confirms my thesis. The solution can actually be further optimised. That being said I am gonna merge and release this as a patch so we can track associated bugs around computed properties. If this strategy resolves them I will do the optimisation refactoring.

Will do this in an hour or two. 👍

[jmyrland]

Awesome!!

This confirms my thesis. The solution can actually be further optimised.

Sweet!

That being said I am gonna merge and release this as a patch so we can track associated bugs around computed properties. If this strategy resolves them I will do the optimisation refactoring.

Will do this in an hour or two. 👍

I was able to update to the alpha now for our apps - but I sadly get alot of failing tests (~50%) 😢

It seems that computed props are undefined in most cases when a value is expected (it worked previously).

I am unable to test the actual app - as I end up in "login"-redirect loop, as we probably are using some computed props in the auth store.

Maybe this is the culprit?

Testcoverage at the current version: 209 of 375 tests passing.

Edit: Removing the performingEqualityCheck condition returning undefined results in total collapse: 97 / 375 tests passing

Edit 2: Tried updating the performingEqualityCheck case to return prevValue - this results in 366 / 375 tests passing.

Edit 3: There is an issue with computed props referencing other computed props. Adding a state resolver for the dependant computed prop seemed to fix the issue. Now 375 / 375 tests passing after this change. I only have 1 failing test suite now, not accounted for in the total number of tests.

Edit 4: The failing test suite was due to a computed prop not having a state resolver. Fixed by adding this; now 379 / 379 tests passing 🎉

This is just the test result of 1 of 6 apps. I think returning the prevValue in this case in stead of undefined might be the solution here. It is however a breaking change, if users of this lib are creating computed props based on other computed props without state resolvers.

[jmyrland]

Actually, using areInputsEqual just for checking inputs and areEqual for checking if the result changed should be sufficient for avoiding rerenders.

I tested this, and it seems to be working. I.e. if this approach is used, it is not a breaking change - and consumers do not have to update their state resolvers.

[ctrlplusb]

Thanks for your help confirming all this!

Can you try easy-peasy@5.1.0-alpha.2?

@jmyrland

[jmyrland]

Thanks for your help confirming all this!

Can you try easy-peasy@5.1.0-alpha.2?

@jmyrland

A total of 1400~ passing unit tests, without changing a single line 👌🎉
And the sandbox still works as expected.

You are a wizard @ctrlplusb ! 😁

[ctrlplusb]

Haha, thanks for the approval @w3bdesign - were you having issues too? Did you try out the alpha release? If so did it resolve your issues?

[ctrlplusb]

Woooooooot!

Thanks so much for having such an extensive test suite for us to leverage!

[jmyrland]

Happy to help! Also, this may close #732

[no-stack-dub-sack]

@jmyrland @ctrlplusb Wow! Thanks for the work on this! This is going to greatly simplify a lot of code and reduce the need for state resolvers when trying to avoid un-wanted re-renders. And yes @jmyrland, from what I can tell, the sandbox is working perfectly with completedTodos and safeCompletedTodos both behaving as expected now.

This is great!