virtual icon indicating copy to clipboard operation
virtual copied to clipboard

feat(solid-virtual): improve Solid adapter

Open martinpengellyphillips opened this issue 1 year ago • 5 comments

I encountered some issues using the Solid adapter with the latest Solid version (1.8). In particular, no items would display unless I added into my code a deferred setting of the element ref (e.g. ref={(el) => queueMicrotask(() => setRef(el))}).

Looking through the source I believe the issue stems from _willUpdate being called with a scrollElement that has not yet been connected to a targetWindow (due to the use of createComputed). This PR attempts to avoid this issue and the need for a workaround in consumer code as well as make a few other changes for optimisation and to maintain reactivity expectations.

--

Use createEffect to ensure scrollElement ref is connected to DOM and ownerDocument/window before attempting to update measurements for it. Otherwise, virtualizer.targetWindow will be set to null (in _willUpdate), the scrollElement rect to nothing and no items will be displayed.

In addition, remove some logic that causes redundant work to be done (e.g. setOptions called multiple times, _willUpdate called on mount which is a no-op if the scollElement didn't change). Instead rely on the options reactivity to perform the bulk of the work.

Also, reset virtual items store when options change to ensure that reactivity is maintained - e.g. so that measureItem is called again in a <For> loop if scrollMargin changed.

martinpengellyphillips avatar Aug 09 '24 21:08 martinpengellyphillips

@martinpengellyphillips big thanks for this, could you add also some test to check the basic stuff 🙏 😉 ?

piecyk avatar Aug 15 '24 15:08 piecyk

@martinpengellyphillips big thanks for this, could you add also some test to check the basic stuff 🙏 😉 ?

@piecyk Sure! Should I follow general approach to tests from the react adapter?

martinpengellyphillips avatar Aug 15 '24 17:08 martinpengellyphillips

@martinpengellyphillips big thanks for this, could you add also some test to check the basic stuff 🙏 😉 ?

@piecyk Sure! Should I follow general approach to tests from the react adapter?

Could be, or you can try playwright, have it on todo for some time. Overall what you feel more comfortable with, we need to start with something.

piecyk avatar Aug 16 '24 07:08 piecyk

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 00fd6fbf73c0adf4cf0f1270c16962319af93722. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

nx-cloud[bot] avatar Aug 22 '24 13:08 nx-cloud[bot]

commit: 00fd6fb

@tanstack/lit-virtual

pnpm add https://pkg.pr.new/@tanstack/lit-virtual@790
@tanstack/react-virtual

pnpm add https://pkg.pr.new/@tanstack/react-virtual@790
@tanstack/solid-virtual

pnpm add https://pkg.pr.new/@tanstack/solid-virtual@790
@tanstack/svelte-virtual

pnpm add https://pkg.pr.new/@tanstack/svelte-virtual@790
@tanstack/virtual-core

pnpm add https://pkg.pr.new/@tanstack/virtual-core@790
@tanstack/vue-virtual

pnpm add https://pkg.pr.new/@tanstack/vue-virtual@790

Open in Stackblitz

More templates

pkg-pr-new[bot] avatar Aug 22 '24 13:08 pkg-pr-new[bot]