[labs/virtualizer] RangeChangedEvent and VisibilityChangedEvent no longer bubble up by usergenic · Pull Request #3609 · lit/lit

usergenic · 2023-01-26T05:30:35Z

This PR solves the event behavior differences between RangeChangedEvent and VisibilityChangedEvent per #3051

Both event types are ensured not to bubble up and must be listened for now on the <lit-virtualizer> itself or the host element in the case of virtualize() directive.

changeset-bot · 2023-01-26T05:30:38Z

🦋 Changeset detected

Latest commit: 0ddcf55

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	Minor

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

github-actions · 2023-01-26T05:30:55Z

📊 Tachometer Benchmark Results

Summary

nop-update

lit-html-kitchen-sink: unsure 🔍 -10% - +7% (-2.58ms - +1.64ms)
^{this-change vs tip-of-tree}

render

lit-element-list: 76.28ms - 81.09ms
lit-html-kitchen-sink: unsure 🔍 -6% - +1% (-1.96ms - +0.22ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -8% - +1% (-0.98ms - +0.07ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -3% - +1% (-1.43ms - +0.56ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -1% - +3% (-0.32ms - +1.76ms)
^{this-change vs tip-of-tree}

update

lit-element-list: 804.41ms - 816.86ms
lit-html-kitchen-sink: unsure 🔍 -3% - +5% (-2.71ms - +4.22ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -3% - +1% (-9.20ms - +2.69ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -2% - +2% (-1.82ms - +1.79ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -1% - +1% (-5.28ms - +5.50ms)
^{this-change vs tip-of-tree}

update-reflect

lit-element-list: 805.25ms - 813.53ms
reactive-element-list: unsure 🔍 -1% - +0% (-8.57ms - +1.92ms)
^{this-change vs tip-of-tree}

Results

lit-element-list

Browser: chrome-headless 109.0.5414.74
Sample size: 50
Built by: Benchmarks #4336
Commit: ec88a37

render

Version	Avg time	vs
	76.28ms - 81.09ms	-

update

Version	Avg time	vs
	804.41ms - 816.86ms	-

update-reflect

Version	Avg time	vs
	805.25ms - 813.53ms	-

lit-html-kitchen-sink

Browser: chrome-headless 109.0.5414.74
Sample size: 30
Built by: Benchmarks #4336
Commit: ec88a37

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	31.87ms - 33.08ms	-	unsure 🔍 -6% - +1% -1.96ms - +0.22ms	unsure 🔍 -3% - +3% -0.92ms - +0.85ms
tip-of-tree tip-of-tree	32.44ms - 34.26ms	unsure 🔍 -1% - +6% -0.22ms - +1.96ms	-	unsure 🔍 -1% - +6% -0.27ms - +1.95ms
previous-release previous-release	31.87ms - 33.15ms	unsure 🔍 -3% - +3% -0.85ms - +0.92ms	unsure 🔍 -6% - +1% -1.95ms - +0.27ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	78.24ms - 83.94ms	-	unsure 🔍 -3% - +5% -2.71ms - +4.22ms	unsure 🔍 -6% - +4% -5.09ms - +3.04ms
tip-of-tree tip-of-tree	78.36ms - 82.31ms	unsure 🔍 -5% - +3% -4.22ms - +2.71ms	-	unsure 🔍 -6% - +2% -5.29ms - +1.73ms
previous-release previous-release	79.22ms - 85.02ms	unsure 🔍 -4% - +6% -3.04ms - +5.09ms	unsure 🔍 -2% - +7% -1.73ms - +5.29ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	22.42ms - 25.74ms	-	unsure 🔍 -10% - +7% -2.58ms - +1.64ms	unsure 🔍 -15% - +7% -3.69ms - +1.91ms
tip-of-tree tip-of-tree	23.25ms - 25.85ms	unsure 🔍 -7% - +11% -1.64ms - +2.58ms	-	unsure 🔍 -12% - +9% -3.02ms - +2.18ms
previous-release previous-release	22.73ms - 27.22ms	unsure 🔍 -8% - +15% -1.91ms - +3.69ms	unsure 🔍 -9% - +12% -2.18ms - +3.02ms	-

lit-html-repeat

Browser: chrome-headless 109.0.5414.74
Sample size: 30
Built by: Benchmarks #4336
Commit: ec88a37

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	11.47ms - 12.25ms	-	unsure 🔍 -8% - +1% -0.98ms - +0.07ms	unsure 🔍 -7% - +3% -0.81ms - +0.32ms
tip-of-tree tip-of-tree	11.96ms - 12.67ms	unsure 🔍 -1% - +8% -0.07ms - +0.98ms	-	unsure 🔍 -3% - +6% -0.33ms - +0.75ms
previous-release previous-release	11.70ms - 12.51ms	unsure 🔍 -3% - +7% -0.32ms - +0.81ms	unsure 🔍 -6% - +3% -0.75ms - +0.33ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	311.93ms - 319.98ms	-	unsure 🔍 -3% - +1% -9.20ms - +2.69ms	unsure 🔍 -1% - +2% -3.87ms - +7.34ms
tip-of-tree tip-of-tree	314.83ms - 323.59ms	unsure 🔍 -1% - +3% -2.69ms - +9.20ms	-	unsure 🔍 -0% - +3% -0.87ms - +10.86ms
previous-release previous-release	310.31ms - 318.12ms	unsure 🔍 -2% - +1% -7.34ms - +3.87ms	unsure 🔍 -3% - +0% -10.86ms - +0.87ms	-

lit-html-template-heavy

Browser: chrome-headless 109.0.5414.74
Sample size: 50
Built by: Benchmarks #4336
Commit: ec88a37

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	53.58ms - 54.99ms	-	unsure 🔍 -3% - +1% -1.43ms - +0.56ms	unsure 🔍 -2% - +2% -1.02ms - +0.83ms
tip-of-tree tip-of-tree	54.02ms - 55.42ms	unsure 🔍 -1% - +3% -0.56ms - +1.43ms	-	unsure 🔍 -1% - +2% -0.59ms - +1.27ms
previous-release previous-release	53.78ms - 54.98ms	unsure 🔍 -2% - +2% -0.83ms - +1.02ms	unsure 🔍 -2% - +1% -1.27ms - +0.59ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	117.96ms - 120.59ms	-	unsure 🔍 -2% - +2% -1.82ms - +1.79ms	unsure 🔍 -2% - +1% -1.84ms - +1.32ms
tip-of-tree tip-of-tree	118.05ms - 120.53ms	unsure 🔍 -2% - +2% -1.79ms - +1.82ms	-	unsure 🔍 -1% - +1% -1.76ms - +1.27ms
previous-release previous-release	118.67ms - 120.41ms	unsure 🔍 -1% - +2% -1.32ms - +1.84ms	unsure 🔍 -1% - +1% -1.27ms - +1.76ms	-

reactive-element-list

Browser: chrome-headless 109.0.5414.74
Sample size: 50
Built by: Benchmarks #4336
Commit: ec88a37

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	52.49ms - 54.10ms	-	unsure 🔍 -1% - +3% -0.32ms - +1.76ms	unsure 🔍 -1% - +3% -0.33ms - +1.78ms
tip-of-tree tip-of-tree	51.91ms - 53.24ms	unsure 🔍 -3% - +1% -1.76ms - +0.32ms	-	unsure 🔍 -2% - +2% -0.95ms - +0.96ms
previous-release previous-release	51.89ms - 53.25ms	unsure 🔍 -3% - +1% -1.78ms - +0.33ms	unsure 🔍 -2% - +2% -0.96ms - +0.95ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	835.32ms - 843.97ms	-	unsure 🔍 -1% - +1% -5.28ms - +5.50ms	unsure 🔍 -1% - +1% -6.37ms - +5.08ms
tip-of-tree tip-of-tree	836.33ms - 842.75ms	unsure 🔍 -1% - +1% -5.50ms - +5.28ms	-	unsure 🔍 -1% - +0% -5.69ms - +4.19ms
previous-release previous-release	836.54ms - 844.04ms	unsure 🔍 -1% - +1% -5.08ms - +6.37ms	unsure 🔍 -0% - +1% -4.19ms - +5.69ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	848.73ms - 857.32ms	-	unsure 🔍 -1% - +0% -8.57ms - +1.92ms	unsure 🔍 -1% - +1% -6.18ms - +4.57ms
tip-of-tree tip-of-tree	853.34ms - 859.36ms	unsure 🔍 -0% - +1% -1.92ms - +8.57ms	-	unsure 🔍 -0% - +1% -1.90ms - +6.94ms
previous-release previous-release	850.59ms - 857.07ms	unsure 🔍 -1% - +1% -4.57ms - +6.18ms	unsure 🔍 -1% - +0% -6.94ms - +1.90ms	-

_{tachometer-reporter-action v2 for Benchmarks}

usergenic · 2023-01-26T19:06:59Z

CC: @Westbrook @najikahalsema @graynorton this PR wraps up concerns discussed in issue #3051 about Virtualizer's event propagation

justinfagnani

LGTM!

The comments are just some musings from not being familiar with the code. This can go in as-is.

justinfagnani · 2023-01-27T02:25:47Z

packages/labs/virtualizer/src/events.ts


  constructor(range: Range) {
-    super(RangeChangedEvent.eventName, {bubbles: true});
+    super(RangeChangedEvent.eventName, {bubbles: false});


Not for this PR, but can we get code comments on the event class and on first and last to document to users and maintainers what they're for? It can just be what's in the readme here: https://github.com/lit/lit/tree/main/packages/labs/virtualizer#visibilitychanged-event

justinfagnani · 2023-01-27T02:33:03Z

packages/labs/virtualizer/src/test/scenarios/element-and-directive-parity.test.ts

  @property({type: Array, attribute: false})
  public items: Array<number> = [];
+
+  @property({type: Array, attribute: false})


type is only used for attribute converters (it would be more accurately named typeHint), so if a property has attribute: false it doesn't need a type at all. For test elements, it's academic though and you could just leave all properties off. The converter would only kick in if reflecting to an attribute (which is off) or setting from an attribute, which you're not doing.

justinfagnani · 2023-01-27T02:43:23Z

packages/labs/virtualizer/src/test/scenarios/element-and-directive-parity.test.ts

      'using-lit-virtualizer'
    )!;
+    const uvd: UsingVirtualizeDirective = example.querySelector(
+      'using-virtualize-directive'


For my edification, do you know why we need to wait for these elements first with the until() call above? The elements are defined in the module, and before the fixture call, which itself is awaited. I wouldn't think the until()s are needed at all. but if there was a need to wait for an upgrade, customElements.whenDefined('using-virtualize-directive') should work too.

I'm asking because the test is so long that any reduction in boilerplate will help new people read it.

Yeah this was how I was watching for upgrades from the outside. I didn't want to select the element until after the upgrade occurred, but I'll see if I can either eliminate the need for those, write a helper, or comment why it's necessary to explicitly await the upgrades for the elements when I do test clean ups in #2788

justinfagnani · 2023-01-27T02:44:32Z

packages/labs/virtualizer/src/test/scenarios/element-and-directive-parity.test.ts


-    expect(uvd.shadowRoot?.textContent).to.include('2 selected');
-    expect(uvd.shadowRoot?.textContent).to.include('5 selected');
+    expect(ulv.shadowRoot?.textContent).not.to.include('[2 selected]');


Maybe not now, but since the test is so long, it'd be great to see some comments break it up and describe the scenario, what the setup is doing, what the interaction is, expectations are checking, etc.

Fair points. I'll add this when I do some overall test sprucing up in #2788

usergenic requested a review from justinfagnani as a code owner January 26, 2023 05:30

usergenic force-pushed the virtualizer-mmm-bop-stop-stopprop branch from 0de6f79 to 1ae6a28 Compare January 26, 2023 05:31

Ensure RangeChangedEvent and VisibilityChangedEvent no longer bubble up.

0ddcf55

usergenic force-pushed the virtualizer-mmm-bop-stop-stopprop branch from 1ae6a28 to 0ddcf55 Compare January 26, 2023 05:33

justinfagnani approved these changes Jan 27, 2023

View reviewed changes

usergenic merged commit 54046b0 into main Jan 27, 2023

usergenic deleted the virtualizer-mmm-bop-stop-stopprop branch January 27, 2023 19:15

lit-robot mentioned this pull request Mar 10, 2023

Version Packages #3717

Merged

usergenic mentioned this pull request Mar 14, 2023

[labs/virtualizer] rangeChanged event stops propagation on itself #3051

Closed

Conversation

usergenic commented Jan 26, 2023

Uh oh!

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

Uh oh!

🦋 Changeset detected

Uh oh!

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

Uh oh!

📊 Tachometer Benchmark Results

Summary

nop-update

render

update

update-reflect

Results

render

update

update-reflect

render

update

nop-update

render

update

render

update

render

update

update-reflect

Uh oh!

usergenic commented Jan 26, 2023

Uh oh!

justinfagnani left a comment

Choose a reason for hiding this comment

Uh oh!

justinfagnani Jan 27, 2023

Choose a reason for hiding this comment

Uh oh!

justinfagnani Jan 27, 2023

Choose a reason for hiding this comment

Uh oh!

justinfagnani Jan 27, 2023

Choose a reason for hiding this comment

Uh oh!

usergenic Jan 27, 2023

Choose a reason for hiding this comment

Uh oh!

justinfagnani Jan 27, 2023

Choose a reason for hiding this comment

Uh oh!

usergenic Jan 27, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

changeset-bot bot commented Jan 26, 2023 •

edited

Loading

github-actions bot commented Jan 26, 2023 •

edited

Loading