Begin diff with next DOM sibling in forceUpdate #1689
Conversation
|
|
||
| /** @jsx h */ | ||
|
|
||
| describe('getDomSibling', () => { |
There was a problem hiding this comment.
@marvinhagemeister FYI, I copied your tests from #1605 and added a bunch. I couldn't get the implementation of getDomSibling in #1605 to pass all the new tests which is why I re-built it from scratch.
There was a problem hiding this comment.
I'm glad you did that as my version was a bit hacked together to get a good sense of the problem space. I didn't have time to clean it up. The variant in this PR is much cleaner and the childIndex handling is ace 💯
| it('should find sibling with nested placeholder', () => { | ||
| render(( | ||
| <div key="0"> | ||
| <Fragment key="0.0"> |
There was a problem hiding this comment.
I added keys to some of the tests here just cuz they made it easier to debug getDomSibling with a couple of well-placed console.log(vnode.key)
There was a problem hiding this comment.
Ohh that's a very neat way to debug ordering. I'll use that on my next PR 👍
| * @param {import('./internal').VNode} vnode | ||
| * @param {number | null} [childIndex] | ||
| */ | ||
| export function getDomSibling(vnode, childIndex) { |
There was a problem hiding this comment.
I forget - does JS support optimized tail recursion? I think this function could be re-written to be tail-recursive if it isn't already (I forget what exactly qualifies as tail recursive).
There was a problem hiding this comment.
V8 had it for a while but not anymore, tracking bug: https://bugs.chromium.org/p/v8/issues/detail?id=4698
There was a problem hiding this comment.
Hey Andre, super nice work! I think this solves the following issue as well: https://github.com/preactjs/preact/pull/1687/files#diff-d5abae78db533876550fd82502fe7000R186
| * @param {import('./internal').VNode} vnode | ||
| * @param {number | null} [childIndex] | ||
| */ | ||
| export function getDomSibling(vnode, childIndex) { |
There was a problem hiding this comment.
V8 had it for a while but not anymore, tracking bug: https://bugs.chromium.org/p/v8/issues/detail?id=4698
There was a problem hiding this comment.
Wohoo! This PR makes me really excited!! It is way better than my initial draft in #1605 👍 👍 Especially the getDomSibling function is a thing of beauty 🙌
Hopefully we've caught all remaining issues with Fragment ordering with this PR. The approach is a lot more sound compared to what's in master. Really amazing things happening here 💯 🍀
andrewiggins
force-pushed
the
forceupdate-diffChildren3
branch
from
97b8726
to
ab51a14
Compare
andrewiggins
force-pushed
the
forceupdate-diffChildren3
branch
from
9dcbd6d
to
cb4e6e7
Compare
When a Component isn't currently associated with a DOM node, we need to begin the diff from it's next DOM sibling so that if this render of the Component adds DOM, it will be correctly inserted into it's parent (instead of always appended like before).
I stole this idea from @marvinhagemeister's #1605, but re-implemented it so it no longer needs a
_siblingpointer which should make this a little smaller.I tried exploring other ideas (like two loops in diffChildren) but they didn't pan out like I had hoped (would be just as big a change and couldn't get Fragments working after a while 😞). I figured it would be better to just get something working in master first.
With #1658, I think this solution works out pretty elegantly anyway (
diffChildrenis still the only place where DOM is added or removed).Total change: +75 B 😬
NOTE: depends on #1688 (+5 B) so don't merge until after #1688 is in master. I'll rebase this PR on master once that happens.