fix(lit-virtual): add setOptions to VirtualizerController for reactive option updates - #1253
fix(lit-virtual): add setOptions to VirtualizerController for reactive option updates#1253waterWang wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 8 reviews per rolling hour; 7 remain after this review. 📝 WalkthroughWalkthrough
ChangesLit virtualizer option updates
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized change updates reactive virtualizer options and adds focused test coverage; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant LitComponent
participant VirtualizerControllerBase
participant Virtualizer
LitComponent->>VirtualizerControllerBase: setOptions(partialOptions)
VirtualizerControllerBase->>Virtualizer: setOptions(mergedOptions)
VirtualizerControllerBase->>Virtualizer: _willUpdate()
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/lit-virtual/tests/index.test.tsParsing error: "parserOptions.project" has been provided for Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
MILLERMARRU
left a comment
MILLERMARRU
left a comment
There was a problem hiding this comment.
This closes an obvious API gap, every other framework adapter (React, Vue, Solid) can update virtualizer options reactively, but lit-virtual's VirtualizerControllerBase only accepted options at construction time. The implementation stores this.options and merges partial updates on setOptions, re-wrapping onChange the same way the constructor does, which keeps the host's requestUpdate callback wired correctly on every option change. Calling this.virtualizer._willUpdate() after setOptions looks necessary and correct, skipping it would leave stale virtual items rendered until the next unrelated re-render. The added test exercises this by calling setOptions inside render(), which is the realistic Lit usage pattern.
|
View your CI Pipeline Execution ↗ for commit fe62dbe ☁️ Nx Cloud last updated this comment at |
piecyk
left a comment
piecyk
left a comment
There was a problem hiding this comment.
Thanks for this — the implementation itself is right. I checked it separately: driving setOptions({ count }) from a reactive property moves getTotalSize() from 500 to 1000 and re-renders the host.
The blocker is the new test, which fails on this branch:
× should apply updated options via setOptions 1013ms
Error: Timeout: Element did not render items beyond initial count of 10
It isn't flaky — the assertion can never pass. Details inline.
Two other things before merge:
Changeset. There isn't one. It should be minor rather than patch, since this adds public API (the repo maps feat → minor, fix → patch). Worth retitling the PR to feat(lit-virtual): to match. Add .changeset/lucky-pugs-shave.md:
---
'@tanstack/lit-virtual': minor
---
Add `setOptions` to `VirtualizerController` and `WindowVirtualizerController` so virtualizer options can be updated reactively after construction.
Previously the controller only accepted options once, in its constructor, so options derived from reactive Lit properties (`count`, `estimateSize`, `horizontal`, …) could never be re-applied — the only escape hatch was `getVirtualizer().setOptions(...)`, which drops the controller's `onChange` wrapper and stops the host from re-rendering.
`setOptions` takes a partial set of options, merges them over the options the controller already holds, re-applies the host `requestUpdate` wrapper around `onChange`, and flushes the change so the host re-renders:
```ts
willUpdate(changed: PropertyValues<this>) {
if (changed.has('count')) {
this.virtualizerController.setOptions({ count: this.count })
}
}
```Docs. docs/framework/lit/lit-virtual.md doesn't mention setOptions. A short section would help, especially to note that the controller merges partial options while virtualizer.setOptions replaces them wholesale — mixing the two leaves the controller's cached this.options stale.
| public setOptions( | ||
| options: Partial<VirtualizerOptions<TScrollElement, TItemElement>>, | ||
| ) { | ||
| this.options = { ...this.options, ...options } |
There was a problem hiding this comment.
A plain spread lets an explicit undefined wipe a stored option: setOptions({ estimateSize: undefined }) leaves estimateSize undefined for good, and core then falls back to its own default rather than the value set earlier.
Core's setOptions guards against exactly this (it skips keys whose value is undefined) — worth matching here.
| options: Partial<VirtualizerOptions<TScrollElement, TItemElement>>, | ||
| ) { | ||
| this.options = { ...this.options, ...options } | ||
| const resolvedOptions: VirtualizerOptions<TScrollElement, TItemElement> = { |
There was a problem hiding this comment.
This resolvedOptions block is a copy of the one in the constructor, and the two close over different things (options vs this.options). Harmless today, but a shared private resolveOptions(options) used by both would stop them drifting.
| } | ||
|
|
||
| render() { | ||
| this.virtualizerController.setOptions({ |
There was a problem hiding this comment.
Calling setOptions inside render() means the count is 20 from the very first paint, so nothing is actually updated reactively — the test would pass without any reactivity at all.
Driving it from a reactive @property in willUpdate is what the feature is for, and it avoids modelling a pattern that mutates controller state during render.
| ) | ||
| await elementUpdated(el) | ||
| await waitUntil( | ||
| () => el.shadowRoot.querySelector('[data-index="15"]'), |
There was a problem hiding this comment.
This is what fails. Index 15 is never rendered: the viewport is 400px with estimateSize: () => 50, so only about indexes 0–9 render at scroll offset 0 — with count: 10 and with count: 20 alike. Raising the count doesn't widen the rendered window, so the test would fail even if setOptions were perfect.
getTotalSize() is what actually moves (500 → 1000). Assert that instead, and assert the initial 500 too, so the test also covers the constructor options — right now nothing checks the starting count of 10 took effect.
The follow-up expect(...).toBeTruthy() on line 197 is also redundant; waitUntil already guarantees it.
3 participants
Description
Fixes #1251
() never calls after the initial construction, so reactive option updates (a changed , , , etc.) never reach the underlying instance.
Root cause
The constructor captures options once and passes them to . The lifecycle method only calls without re-applying options. Every other framework adapter (, ) calls before every .
Fix
Added method to — stores the options on the instance, merges with new partial options, and calls + . Matches the pattern used by 's and 's injectable.
Stored as a private field — enables the method to merge partial updates with the base options (including defaults from and subclass constructors).
Consumer usage
Changes
Summary by CodeRabbit
New Features
Tests