-
-
Notifications
You must be signed in to change notification settings - Fork 963
reconcile: support "merge" in objects #2475
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
Comments
I did some performance testing on my realistic workload. spread - 0.275ms
for loop no batch - 0.205ms
for loop with batch - 0.225ms
I think the difference is so extremely small, that I'll stick with the spread as it has the cleanest syntax. I also did a run in a 10k for loop, and got 370 ms for spread and 240 ms for batch and no-batch. I mean that's for a 10k loop! Basically this is so extremely fast for a single call, that there is no point of optimising for performance. |
I agree this is sort of loose territory. We have some big change to store reconcile in 2.0. Mainly there is no The delete loop is necessary for clearing stores... it's actually the only way to reset them. That being said there are more internal changes in reconcile coming in this as well. It's unlikely we change anything in 1.0. As you said you can reconcile deeper along the path or clone if you want shallowly which is inexpensive. |
I'm happy to wait for 2.0 to have some improvements. The other common issue is Date() objects. Right now, setStore + reconcile handles every common types (both basic and wrappable ones), except Date() objects. There is no way to set a Date() object without triggering an update. So, for both of these cases, I ended up making this utility function for my personal use. It does partial reconcile and proper Date comparison, using
|
Yeah for non-primitive values it is going to look at the reference. I'm not sure if there is much anything to do for that. We've been looking at wrapping anything you stick in the store by default.. but that isn't going to help here I think because again new instance of date. |
What about adding a special case for Date()? With Date() you'd cover 99% of the most common types. Or, adding a type -> eqFn map like For the partial part, I think reconcile/setState needs to make a difference between 1st level and 2+ levels. Right now it's a beautiful solution, but it doesn't handle the most basic use case very well, that is to differentiate between the store object and a subkey. I mean for a subkey, you definitely want deletion, but for the full store object, it's very common to do partial updates. I recommend some kind of differentiation for store vs. subkey in the 2.0 version. |
A common pattern I believe is to reconcile partial state updates in stores.
For example, part of a store is synced to a backend, so after a push-to-backend operation, it returns an up-to-date data for part of the store values. Other values, like UI related ones are not included in this data.
Passing this raw data into setStore works, but triggers unnecessary updates. It triggers every single key's update, which is an array or object.
Here is my 1st idea to tackle this: running reconcile in a for loop on each 1st level key:
This works, and when put in a batch() should be performant. Quite a lot of code, so asks for a wrapper function, but should be good.
My 2nd idea is this:
This also works. If I understand correctly, it's a tiny bit slower, but the slowness happens in the pure JS code, not in the actual UI updates, so it should still be fast.
When I looked at the source code for reconcile and found that it supports merge, but only for arrays.
I'd like to recommend to add support for merge in objects. To make sure that this does not modify the array behaviours in sub-keys, it should be called something different, "partial" probably.
I believe it'd be as simple as turning off the delete loop in the first level of applyState.
Please tell me if I'm not correct in this. Isn't this a common pattern when using stores?
How do you normally solve this?
The text was updated successfully, but these errors were encountered: