fix: delete from batch_values on updates #17115
Conversation
This fixes a bug where a derived would still show its old value even after it was indirectly updated again within the same batch. This can for example happen by reading a derived on an effect, then writing to a source in that same effect that makes the derived update, and then read the derived value in a sibling effect - it still shows the old value without the fix. The fix is to _delete_ the value from batch_values, as it's now the newest value across all batches. In order to not prevent breakage on other batches we have to leave the status of deriveds as-is, i.e. within is_dirty and update_derived we cannot update its status. That might be a bit more inefficient as you now have to traverse the graph for each `get` of that derived (it's a bit like they are all disconnected) but we can always optimize that later if need be. Another advantage of this fix is that we can get rid of duplicate logic we had to add about unmarking and reconnecting deriveds, because that logic was only needed for the case where deriveds are read after they are updated, which now no longer hits that if-branch
|
|
| // TODO if that turns out to be a performance problem, we could try | ||
| // to save the current status of the derived in a map and restore it | ||
| // before leaving the batch. | ||
| batch_values.delete(derived); |
There was a problem hiding this comment.
This feels redundant, since we never do batch_values.set(derived, ...) in the first place. If I delete this and the other batch_values?.delete occurrence in batch.js, all the tests still pass.
So basically what this PR is doing is removing caching for deriveds altogether when time-travelling. It eliminates any problems around staleness, but it feels like quite a high cost
There was a problem hiding this comment.
We did set prior to this change, and a writable derived would end up in batch_values via internal_set.
Your adjustments should still make it better since we can now stop at the changed derived. Wonder if we can then also remove the && batch_values === null I added in is_dirty
This fixes a bug where a derived would still show its old value even after it was indirectly updated again within the same batch. This can for example happen by reading a derived on an effect, then writing to a source in that same effect that makes the derived update, and then read the derived value in a sibling effect - it still shows the old value without the fix.
The fix is to delete the value from batch_values, as it's now the newest value across all batches. In order to not prevent breakage on other batches we have to leave the status of deriveds as-is, i.e. within is_dirty and update_derived we cannot update its status. That might be a bit more inefficient as you now have to traverse the graph for each
getof that derived (it's a bit like they are all disconnected) but we can always optimize that later if need be.Another advantage of this fix is that we can get rid of duplicate logic we had to add about unmarking and reconnecting deriveds, because that logic was only needed for the case where deriveds are read after they are updated, which now no longer hits that if-branch