Improve Solid adapter. #790
Conversation
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.
| virtualizer.setOptions( | ||
| mergeProps(resolvedOptions, options, { | ||
| onChange: ( | ||
| instance: Virtualizer<TScrollElement, TItemElement>, | ||
| sync: boolean, | ||
| ) => { | ||
| instance._willUpdate() | ||
| setVirtualItems( | ||
| reconcile(instance.getVirtualItems(), { | ||
| key: 'index', |
There was a problem hiding this comment.
Should also make this 'key' rather than 'index' so that it uses any custom supplied key from getItemKey option for the diff.
| virtualizer.setOptions( | ||
| mergeProps(resolvedOptions, options, { | ||
| onChange: ( | ||
| instance: Virtualizer<TScrollElement, TItemElement>, | ||
| sync: boolean, | ||
| ) => { | ||
| instance._willUpdate() | ||
| setVirtualItems( |
There was a problem hiding this comment.
Should batch the set state calls together.
|
@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. |
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 00fd6fb. 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 targetsSent with 💌 from NxCloud. |
| virtualizer.setOptions( | ||
| mergeProps(resolvedOptions, options, { | ||
| onChange: ( | ||
| instance: Virtualizer<TScrollElement, TItemElement>, | ||
| sync: boolean, | ||
| ) => { | ||
| instance._willUpdate() | ||
| setVirtualItems( | ||
| reconcile(instance.getVirtualItems(), { | ||
| key: 'index', |
| virtualizer.setOptions( | ||
| mergeProps(resolvedOptions, options, { | ||
| onChange: ( | ||
| instance: Virtualizer<TScrollElement, TItemElement>, | ||
| sync: boolean, | ||
| ) => { | ||
| instance._willUpdate() | ||
| setVirtualItems( |
| @@ -81,6 +78,8 @@ function createVirtualizerBase< | |||
| }, | |||
| }), | |||
| ) | |||
| setVirtualItems([]) | |||
| virtualizer._willUpdate() | |||
| virtualizer.measure() | |||
There was a problem hiding this comment.
Btw the measure here looks suspicious
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
_willUpdatebeing called with ascrollElementthat has not yet been connected to atargetWindow(due to the use ofcreateComputed). 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
createEffectto ensure scrollElement ref is connected to DOM and ownerDocument/window before attempting to update measurements for it. Otherwise,virtualizer.targetWindowwill 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.
setOptionscalled multiple times,_willUpdatecalled 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
measureItemis called again in a<For>loop ifscrollMarginchanged.