handle moving nodes #382
handle moving nodes #382
Conversation
| @@ -460,7 +460,6 @@ function insertExpression(parent, value, current, marker, unwrapArray) { | |||
| if (value === current) return current; | |||
| const t = typeof value, | |||
| multi = marker !== undefined; | |||
| parent = (multi && current[0] && current[0].parentNode) || parent; | |||
There was a problem hiding this comment.
Not sure what might break by this removal but this for sure breaks the parent comparison in cleanChildren
There was a problem hiding this comment.
Ryan made a comment about this code in a related issue here #307 (comment)
I will have to study it again to refresh my mind, I'm not sure I actually ever understood that part
There was a problem hiding this comment.
I've tried to track this down and, oh well 😅, no wonder even Ryan can't remember this. This check existed since the beginning.
There was a problem hiding this comment.
Yeah someone made a codesandbox that was sorting nodes around randomly on an interval and it would occasionally break. I added that way back. But I think its need predates the "modern" way we handle fragments I changed to back in Feb 2019.
In any case I'm moving forward with this not there right now in 2.0 stuff.
| if (!moved) { | ||
| moved = new WeakSet(); | ||
| queueMicrotask(() => (moved = null)); | ||
| } |
There was a problem hiding this comment.
WeakSet is already used by hydration so doesn't hurt anything but there might be better approaches than this.
There was a problem hiding this comment.
I suggest considering a strong referenced array to be cleared in the microtask as moved.length = 0. As the cost of instantiating could be removed, and also a set will try to deduplicate the nodes
There was a problem hiding this comment.
I was thinking if weakset was faster than array to check for element existence. Any form of book-keeping works really. We could also keep it in the nodes but I don't know the performance side of this. Essentially we need to know if a node has changed position in the tree.
There was a problem hiding this comment.
Makes sense. Last time I measured this, for a fairly low amount of items there was no significant difference between [].includes and set.has and you do not pay the cost of the Set having to deduplicate/hash items. I think solid reactivity does this (ex: reading a signal just pushes to observers without deduplicating).
At first sight sounds like keeping the info in the node will complicate the logic, either a microtask for each node or some sort of counter. Probably worth considering, as a microtask could introduce desync, not sure if solidjs/signals will use microtasks, but everything becomes a race when used. This is why I do not like reactivity in microtasks
Maybe make of the WeakSet a Set to be hold in the parent scope, and call set.clear() in the microtask. I'm thinking of at least not paying the cost of creating the set/array each time.
Signed-off-by: Nyi Nyi Lwin <[email protected]>
|
In general i'm interested in this but it is likely not the sort of thing I can see being changed in Solid 1.0 lifespan. Already making moves towards 2.0's structure which gives us ample time to performance benchmark and vet if this makes sense. What this means for the Dom Expressions project is more than likely a bit of a jump in version 0.40.0 we can look at including it there. It might be possible that it is time for DOM Expressions to go to 1.0 given how big of a change is going on. |
fixes solidjs/solid#2357