Fix more remote node reordering issues #161
Conversation
|
@alxclark Could you give a brief description of the issues you were hitting? I'm curious if some of the bugs we're seeing at Stripe are related. |
There was a problem hiding this comment.
If it works, ship it 🤷
It hurts my brain but thanks for adding tests. To aid in future refactoring (as you mentioned), it would help to have solid test coverage. I commented on some bits that look to be missing test coverage, in case you have time to add more tests.
The parts I didn't comment on seemed to have test coverage 🚀
| existingAttached = attachedNodes.get( | ||
| existingId ?? ROOT_ID, | ||
| ) as RemoteReceiverAttachableRoot; |
There was a problem hiding this comment.
This case doesn't seem to have test coverage. Is it easy to add?
| if (id !== existingId) { | ||
| existingAttached.version += 1; | ||
| enqueueUpdate(existingAttached); | ||
| } |
There was a problem hiding this comment.
I don't see any test failures when commenting this out. Maybe there is a way to cover it?
| normalizedChild = normalizeNode(child, addVersion); | ||
| retain(normalizedChild); | ||
| attach(normalizedChild); |
There was a problem hiding this comment.
I don't see test failures when commenting this out, either
| ? Object.freeze(currentChildren) | ||
| : currentChildren; | ||
|
|
||
| newChildren = [...internals.children]; |
There was a problem hiding this comment.
I don't see test failures when commenting this out
| ? Object.freeze(currentChildren) | ||
| : currentChildren; | ||
|
|
||
| newChildren = [...internals.children]; |
There was a problem hiding this comment.
I don't see test failures when commenting this out
|
@ericfrank-stripe When reordering components on a remote when an explicit |
|
@kumar303 for sure, the core library is missing a lot of tests. We have mostly just used the minimal integration tests in |
My previous fix in #160 only covered a subset of the reordering bugs in remote-ui. There were two other issues that caused problems in other cases:
appendChildBefore()in cases where the child was moving to a larger index inchildrenin the same parent as it was already attachedRemoteRootobject.This PR fixes both of those issues. The code is a little gnarly and I feel like I could do the whole "map remote root to remote receiver" much more cleanly if I started from scratch, but it appears to fix the reordering bugs that the last PR did not.