[labs/virtualizer] Simplify initialization, more complete fix for #3481 by graynorton · Pull Request #3624 · lit/lit

graynorton · 2023-01-31T22:19:39Z

No longer use events to communicate between layout and virtualizer
Remove dependency on EventTarget polyfill
No longer auto-load ResizeObserver polyfill (necessitating major version bump)
Init is now entirely sync, except for auto-loading of default layout as needed
More complete fix for [labs/virtualizer] Error when immediately re-rendering (before async connect code runs) #3481 (handle immediate layout change)

…xcept for default layout

…gression

changeset-bot · 2023-01-31T22:19:43Z

🦋 Changeset detected

Latest commit: 6438670

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@lit-labs/virtualizer	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

usergenic · 2023-02-01T02:40:44Z

@graynorton I'll look at publishing the ResizeObserver polyfill and removing the included loader and version entirely in a future version, but I like the steps this takes towards that. And there's ResizeObserver polyfill+native behavior tests to boot! Nice!

I think I understand the move away from CustomEvents being dispatched by the layouts to an explicit Layout Message channel too as it is more intention-revealing. Makes it clearer where they're coming from and why.

usergenic

LGTM!

packages/labs/virtualizer/README.md

justinfagnani · 2023-02-01T04:02:02Z

packages/labs/virtualizer/README.md

+
+async function load() {
+  await loadPolyfillIfNeeded();
+  await import('@lit-labs/virtualizer');


It might be instructive to show this happening at the app level, like before dynamic importing the main app code in a bootstrap script. You could use top-level await there.

I considered that briefly, but was struggling to do so in a way that didn't seem overly precise / prescriptive or make too many assumptions about typical app structure, so I ended up falling back to this minimal, more conceptual sketch. Feel free to propose (or just go ahead and make) a better example.

justinfagnani · 2023-02-01T04:08:54Z

packages/labs/virtualizer/src/Virtualizer.ts

  private _pendingLayoutComplete: number | null = null;

+  /**
+   * Layout initialization is async because we dynamically load


How big is the default layout? Could we just statically import it?

Can dig into the size numbers and reconsider the tradeoffs here, but the default flow layout is non-trivial in size, and previous thinking was that it was definitely worth not forcing folks to pay that cost if they were using an alternative layout.

Another option, which we actually briefly tried out at some point in the 0.x history, is to always require a layout to be explicitly loaded and provided by the app. The DevEx hit for the 80% case was unfortunate, though, so we reverted to dynamically auto-loading.

At this point, with all of the async polyfill stuff out of the picture and some kinks worked out, I think we've converged on a dynamic loading scheme that isn't too heavy and shouldn't cause much trouble going forward, so my inclination is to keep this as-is for now.

graynorton · 2023-02-01T18:35:06Z

I think I understand the move away from CustomEvents being dispatched by the layouts to an explicit Layout Message channel too as it is more intention-revealing. Makes it clearer where they're coming from and why.

@usergenic Yeah, I think it is clearer and more direct (and we weren't really benefiting from the indirection), but the main reason I did it as part of this particular PR was to eliminate the use of EventTarget and the associated polyfill. In doing this at the same time as we stop auto-loading the ResizeObserver polyfill, we get rid of all of the async initialization code except for the default layout loading.

usergenic · 2023-02-01T19:44:20Z

I'm not certain why the tests-local and benchmarks checks are failing. The failures are in the localize examples /home/runner/work/lit/lit/packages/localize/examples/transform-js/rollup.config.mjs is importing @rollup/plugin-typescript which "can't be found".

I don't get that error running any builds/tests locally. As soon as I can get the check passing I'll get this merged in.

packages/labs/virtualizer/README.md

43081j · 2023-02-01T22:29:35Z

packages/labs/virtualizer/src/layouts/shared/BaseLayout.ts

+      scrollSize: {
+        [this._sizeDim]: this._scrollSize,
+        [this._secondarySizeDim]: null,
+      } as Size,


tiny nit: it seems the virtualiser actually handles nullish dimensions. is it worth updating Size to reflect that and drop the cast here?

in the virtualiser it already checks size.width !== null etc even though the type says it can't ever be null

near enough a non-issue but maybe still worth me pointing out

This is a good observation; however, I think it might be a longer thread than we want to pull on as part of this PR. The Size type is used in quite a few places in the Virtualizer code base, and I am quite certain that in some of them the logic doesn't anticipate or guard against null values. My inclination is to take care of this independently of this PR, perhaps as part of a more systematic review of internally used types. @usergenic, thoughts?

@graynorton @43081j I'm all for considering making Size nullable as separate PR. It will give time to fully vet the change; there's enough going on here already.

Yup of course. Didn't mean to distract at all. Was mostly an observation 👍

usergenic · 2023-02-02T03:22:11Z

This test failure is persistent now on CI but is not happening on my local machine which indicates a possible issue hidden by environment characteristics. I'll look to see if test can be adjusted or will add a .skip and comment until such adjustment possible

test/scenarios/element-and-directive-parity.test.js:

 ❌ lit-virtualizer and virtualize directive > both render changes based on non-item data changes
      AssertionError: expected 2 to equal 1
      + expected - actual
      
      -2
      +1
      
      at o.<anonymous> (test/scenarios/element-and-directive-parity.test.js:[156](https://github.com/lit/lit/actions/runs/4070763955/jobs/7012246645#step:8:157):50)

…event to adjust to inconsistent behavior in different runs/environments.

usergenic · 2023-02-02T04:22:38Z

Have adjusted the expectation in the test from an equal to a greaterThanOrEqual as it was been expecting a number of events that may not be completely deterministic since RangeChangedEvents are emitted in reaction to environmental factors like resize observers etc and timing differences may play a role. Check is passing now :)

…er README.

graynorton added 7 commits January 31, 2023 14:12

add test for immediate layout change

e8bd06e

consolidate state events

76bcdbe

replace event-based communication between layout and virtualizer

314f11d

no longer auto-load ResizeObserver polyfill; init now entirely sync e…

103f6c5

…xcept for default layout

add fix for immediate layout change

bd7918e

restore separate message for visibility change only, to avoid perf re…

7eb5f98

…gression

update package-lock.json

3c10d46

graynorton requested a review from justinfagnani as a code owner January 31, 2023 22:19

augustjk requested a review from usergenic February 1, 2023 01:05

usergenic approved these changes Feb 1, 2023

View reviewed changes

justinfagnani reviewed Feb 1, 2023

View reviewed changes

This was referenced Feb 1, 2023

[labs/virtualizer] Type error thrown just after the first import after page reload. #3626

Open

[labs/observers] Consider adding polyfill for ResizeObserver #3622

Closed

43081j reviewed Feb 1, 2023

View reviewed changes

packages/labs/virtualizer/README.md Outdated Show resolved Hide resolved

43081j reviewed Feb 1, 2023

View reviewed changes

graynorton and others added 2 commits February 1, 2023 15:41

fix typo in README

11ffb77

Update root package-lock and remove virtualizer specific package-lock

95bb8dc

Updated the range changed events expectation to accept more than one …

6284ff8

…event to adjust to inconsistent behavior in different runs/environments.

Updated section about the forked ResizeObserver polyfill in virtualiz…

6438670

…er README.

usergenic merged commit e51ff22 into lit:main Feb 2, 2023

lit-robot mentioned this pull request Mar 10, 2023

Version Packages #3717

Merged

justinfagnani mentioned this pull request Aug 20, 2025

[labs/virtualizer] Error when immediately re-rendering (before async connect code runs) #3481

Open

Conversation

graynorton commented Jan 31, 2023

Uh oh!

changeset-bot bot commented Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

usergenic commented Feb 1, 2023

Uh oh!

usergenic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

justinfagnani Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

graynorton Feb 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinfagnani Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

graynorton Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

graynorton commented Feb 1, 2023

Uh oh!

usergenic commented Feb 1, 2023

Uh oh!

Uh oh!

43081j Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

graynorton Feb 1, 2023

Choose a reason for hiding this comment

Uh oh!

usergenic Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

43081j Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

usergenic commented Feb 2, 2023

Uh oh!

usergenic commented Feb 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

changeset-bot bot commented Jan 31, 2023 •

edited

Loading

graynorton Feb 1, 2023 •

edited

Loading