(fix) - consistent order of state updates#1534
Conversation
# Conflicts: # src/diff/children.js
andrewiggins
left a comment
There was a problem hiding this comment.
It makes sense to me if the perf got better because rerenders are more performed more optimally. If you invalidate a VNode at the top of the tree that has a lot of children that also queued up rerenders, we'll just rerender the tree once starting at the top instead of rerendering each child then the parent (which again rerenders the child).
| /** Flush the render queue by rerendering all queued components */ | ||
| function process() { | ||
| let p; | ||
| q.sort((a, b) => a._depth - b._depth); |
There was a problem hiding this comment.
Hmm we should consider splicing q (e.g. q.splice(0) or something similar) so that any setState calls queued up in forceUpdate don't get added to q after it has been sorted. I believe that any setState calls invoked while processing a previous setState should be rerendered after the current setState finished (i.e. should be added at the "end" of the queue, which for us means processed in another pass through the queue).
There was a problem hiding this comment.
Gonna see what the react behavior is here and perhaps that can become a test?
Here is what I was playing around with, not sure how helpful it is lol: https://codesandbox.io/s/vv6z0m5k9l
There was a problem hiding this comment.
I have added this change, good spot. Makes me wonder if we should propagate depth in diffChildren aswell....
There was a problem hiding this comment.
With splice it seems to fail a bunch of tests: https://travis-ci.org/developit/preact/builds/519564147
There was a problem hiding this comment.
I had a go at this, but it seems to be a bit more involved. As far as I understand splicing the queue would demand a way to know the cutoff point at which the first item that was not part of the original queue is processed, so that we can sort the second update individually.
Another way could be to append components to the queue as as usual, but to resort it before the next call to pop() in case there were any new items added.
The third way I can come up with is to use a separate array all together. That is my least preferred option, because it can get really tricky when a component is in both queues and so on.
But then again I'm wondering if we don't already support that. Whenever we trigger an update and something calls forceUpdate, like during a lifecycle, we immediately update the component's children. Those are then flagged via c._dirty so that the queue will skip processing them. I'm probably missing something, but to me it seems to work out well (at least in my head).
Either way I feel like this requires more thought and would be better to work on in another PR. It will likely brew some discussions and that would leave the issue open for longer.
|
Thank you for the extra pair of eyes @marvinhagemeister, I think this is ready for review now I removed the residual _depth |
marvinhagemeister
left a comment
There was a problem hiding this comment.
This was a very interesting issue to fix. I'm glad we all found it and it shows that some bugs require a team effort 👍 💯
This fixes the problem that got added in demo, where the order of setting state mattered.
In one case where we first altered the list and then altered the key some things wouldn't properly unmount and caused a loss of focus.
To test: The performance benchmarks of
yarn test:karma:watchseem to have improved quite a lot on my machine. Could someone else test this aswell?Note that I am still looking to make a test for this.
Fixes: #1524 , Fixes #874
Adds: 34B