Svelte: optimize observeIntersection#63816
Conversation
bf47611 to
7f0a9f4
Compare
There was a problem hiding this comment.
WeakMap is used to avoid holding onto a reference to elements that would otherwise be garbage collected.
There was a problem hiding this comment.
This is all getting changed in the upcoming usages PR, so I didn't bother fixing it for this. It just means things get highlighted when intersecting with the root element rather than their scroll container (which actually doesn't matter in this case because they share a bottom boundary)
| <StreamingProgress {state} progress={$stream.progress} on:submit={onResubmitQuery} /> | ||
| </aside> | ||
| <div class="result-list" bind:this={resultContainer}> | ||
| <div class="result-list" bind:this={$resultContainer}> |
There was a problem hiding this comment.
TIL it works to just bind directly to a writable
observeIntersection
| <div bind:this={root} use:observeIntersection on:intersecting={event => (visible = event.detail)} class="matches"> | ||
| <div | ||
| bind:this={root} | ||
| use:observeIntersection={$scrollContainer} |
There was a problem hiding this comment.
I passed in the scroll container to maintain the same behavior here, but it might be cleaner to just not bother with the scroll container and fall back to the root element.
| export function capture(): SearchResultsCapture { | ||
| return resultContainer?.scrollTop ?? 0 | ||
| return $resultContainer?.scrollTop ?? 0 | ||
| } | ||
|
|
||
| export function restore(capture?: SearchResultsCapture): void { | ||
| if (resultContainer) { | ||
| resultContainer.scrollTop = capture ?? 0 | ||
| if ($resultContainer) { | ||
| $resultContainer.scrollTop = capture ?? 0 | ||
| } |
There was a problem hiding this comment.
Binding doesn't happen until the component is mounted, which means that when this script is first run, resultContainer is null, which means the correct typing is HTMLElement | null rather than HTMLElement. That makes this a little more awkward, but having a technically-incorrect type seemed worse.
c65e8b2 to
5ac65b3
Compare
vovakulikov
left a comment
There was a problem hiding this comment.
I remember I had the same issue in tooltip tether logic, it was fine though since
it was just one element where I run this logic with finding scrollable block.
Another solution here would be to change our approach with visibility and switch to the real virtual scroll behavior. But probably this change it's too big at this stage, LGTM
Thanks
While attempting to use
observeIntersectionwith the new references panel, I was running into performance issues with large lists of references (the API does not actually paginate yet). When I took a look at the profile, a good chunk of the time came from finding the nearest scrolling ancestor, specifically theoverflowYpart, which requires computing large chunks of the layout.So this changes the
observeIntersectionaction to take an explicit target container so we don't need to do any searching for a scroll container. For the example that I was debugging, this reduced the time it took to render the list ~5x. Additionally, it establishes a cache for all createdIntersectionObservers rather than just the root observer.Before profile (5.5s):
After profile (1.1s):
Test plan
Tested that scrolling still triggers highlighting in the places this is used, and that the performance impact is much lower for large lists (screenshots above).