Keep track of childDom between diffChildren calls#1515
Conversation
This doesn't break the correctness of the current Fragment behavior but does regress what DOM operations occur
| '<ol>345012.appendChild(<li>3)' | ||
| '<ol>012345.insertBefore(<li>4, <li>0)', | ||
| '<ol>401235.insertBefore(<li>5, <li>0)', | ||
| // TODO: see issue #193 - Hmmm why does this extra append happen? |
There was a problem hiding this comment.
TODO: Migrate issue 193 from old repo and integrate with issue mentioned in description
563fc8d to
b8bd7b4
Compare
marvinhagemeister
left a comment
There was a problem hiding this comment.
This is outstanding work, very impressed here 👍 💯 Passing the next dom sibling through diff is a clever solution. Like you mentioned in your comment we could check if inlining getFirstChildDom is worth it to save some bytes. If it turns out to be too difficult I'd be happy to merge this PR as is. We can iterate on it further on master.
Again, super excited about this work! That's excellent stuff 👍 👍
|
I wonder if this is related to my #1524, maybe the hooks part is something of a red-herring? I'm not using fragments, but it does seem pertain to reconciliation between children. UPDATE: tested this branch locally, does not fix my apparently-unrelated issue. Sorry for the noise! |
|
@natevw No worries! Thanks for taking the time to see if this fixes your issue. We'll have to keep looking to find the fix. |
|
I've been looking into it, I've gotten pretty far but it'll be for this weekend probably. |
marvinhagemeister
left a comment
There was a problem hiding this comment.
Super awesome! Both @andrewiggins and I tried golfing this one down and it should theoretically be possible to do so. Problem is that we always seem to break some tests and seem to need the sparking idea to trim down on size. But because this PR fixes some high priority issues that users have run into, I think it's ready to be merged.
Improve Fragment correctness by keeping track of the current DOM position being diff'ed between calls to
diffChildren.Note: while this does fix a lot of Fragment correctness issues, in some cases it doesn't optimally operate on the DOM and bails out to re-appending all Fragment children. This is a known issue (I'll open that issue) that I'd like to address in a future PR (as it will likely require more/larger changes to
diff/diffChildren).b8bd7b4: +72 B 😬26c359b: +73 B 😬
Fixes #1325
Fixes #1326
Fixes #1415