[lit-html] Remove some redundant code from removePart() #4975

justinfagnani · 2025-04-14T18:50:28Z

Also fixes a directive-helpers test that was incorrectly constructing its test case.

I noticed that removePart() was almost the same as ChildPart.$_clear() with the exception of removing the start node. I also optimized ChildPart.$_clear() a little and noticed that the insertPart() test was wrong.

Also fixes a directive-helpers test that was incorrectly constructing its test case.

changeset-bot · 2025-04-14T18:50:32Z

🦋 Changeset detected

Latest commit: bc5514d

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

This PR includes changesets to release 1 package

Name	Type
lit-html	Patch

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 · 2025-04-14T18:50:39Z

📊 Tachometer Benchmark Results

Summary

nop-update

this-change, tip-of-tree, previous-release: unsure 🔍 -5% - +14% (-0.59ms - +1.70ms)
^{this-change vs tip-of-tree}

render

this-change: 44.54ms - 57.17ms
this-change, tip-of-tree, previous-release: slower ❌ 1% - 7% (0.18ms - 1.27ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +2% (-0.35ms - +0.62ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -12% - +51% (-4.78ms - +23.16ms)
^{this-change vs tip-of-tree}

update

this-change: 512.68ms - 520.88ms
this-change, tip-of-tree, previous-release: unsure 🔍 -3% - +6% (-1.11ms - +2.19ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +3% (-1.72ms - +2.24ms)
^{this-change vs tip-of-tree}
this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +0% (-11.41ms - +2.09ms)
^{this-change vs tip-of-tree}

update-reflect

this-change: 504.35ms - 511.68ms
this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +1% (-4.60ms - +4.43ms)
^{this-change vs tip-of-tree}

Results

this-change

Browser: chrome-headless 135.0.0.0
Sample size: 50
Built by: Benchmarks #6731
Commit: bc5514d

render

Version	Avg time	vs
	44.54ms - 57.17ms	-

update

Version	Avg time	vs
	512.68ms - 520.88ms	-

update-reflect

Version	Avg time	vs
	504.35ms - 511.68ms	-

this-change, tip-of-tree, previous-release

Browser: chrome-headless 135.0.0.0
Sample size: 30
Built by: Benchmarks #6731
Commit: bc5514d

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	19.18ms - 19.89ms	-	slower ❌ 1% - 7% 0.18ms - 1.27ms	unsure 🔍 -1% - +5% -0.15ms - +0.96ms
tip-of-tree tip-of-tree	18.39ms - 19.23ms	faster ✔ 1% - 6% 0.18ms - 1.27ms	-	unsure 🔍 -5% - +1% -0.92ms - +0.28ms
previous-release previous-release	18.71ms - 19.55ms	unsure 🔍 -5% - +1% -0.96ms - +0.15ms	unsure 🔍 -1% - +5% -0.28ms - +0.92ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	38.23ms - 40.33ms	-	unsure 🔍 -3% - +6% -1.11ms - +2.19ms	unsure 🔍 -2% - +6% -0.68ms - +2.40ms
tip-of-tree tip-of-tree	37.46ms - 40.01ms	unsure 🔍 -6% - +3% -2.19ms - +1.11ms	-	unsure 🔍 -4% - +5% -1.39ms - +2.02ms
previous-release previous-release	37.29ms - 39.55ms	unsure 🔍 -6% - +2% -2.40ms - +0.68ms	unsure 🔍 -5% - +4% -2.02ms - +1.39ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	12.36ms - 14.05ms	-	unsure 🔍 -5% - +14% -0.59ms - +1.70ms	unsure 🔍 -4% - +15% -0.46ms - +1.87ms
tip-of-tree tip-of-tree	11.87ms - 13.42ms	unsure 🔍 -13% - +4% -1.70ms - +0.59ms	-	unsure 🔍 -8% - +10% -0.97ms - +1.27ms
previous-release previous-release	11.69ms - 13.30ms	unsure 🔍 -14% - +3% -1.87ms - +0.46ms	unsure 🔍 -10% - +8% -1.27ms - +0.97ms	-

this-change, tip-of-tree, previous-release

Browser: chrome-headless 135.0.0.0
Sample size: 50
Built by: Benchmarks #6731
Commit: bc5514d

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	35.19ms - 35.92ms	-	unsure 🔍 -1% - +2% -0.35ms - +0.62ms	unsure 🔍 -1% - +2% -0.35ms - +0.62ms
tip-of-tree tip-of-tree	35.10ms - 35.74ms	unsure 🔍 -2% - +1% -0.62ms - +0.35ms	-	unsure 🔍 -1% - +1% -0.44ms - +0.45ms
previous-release previous-release	35.11ms - 35.73ms	unsure 🔍 -2% - +1% -0.62ms - +0.35ms	unsure 🔍 -1% - +1% -0.45ms - +0.44ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	77.34ms - 80.06ms	-	unsure 🔍 -2% - +3% -1.72ms - +2.24ms	unsure 🔍 -2% - +3% -1.24ms - +2.56ms
tip-of-tree tip-of-tree	77.00ms - 79.88ms	unsure 🔍 -3% - +2% -2.24ms - +1.72ms	-	unsure 🔍 -2% - +3% -1.56ms - +2.36ms
previous-release previous-release	76.71ms - 79.37ms	unsure 🔍 -3% - +2% -2.56ms - +1.24ms	unsure 🔍 -3% - +2% -2.36ms - +1.56ms	-

this-change, tip-of-tree, previous-release

Browser: chrome-headless 135.0.0.0
Sample size: 50
Built by: Benchmarks #6731
Commit: bc5514d

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	46.68ms - 67.79ms	-	unsure 🔍 -12% - +51% -4.78ms - +23.16ms	unsure 🔍 -25% - +29% -13.83ms - +16.10ms
tip-of-tree tip-of-tree	38.89ms - 57.19ms	unsure 🔍 -38% - +6% -23.16ms - +4.78ms	-	unsure 🔍 -37% - +9% -22.06ms - +5.95ms
previous-release previous-release	45.49ms - 66.70ms	unsure 🔍 -28% - +24% -16.10ms - +13.83ms	unsure 🔍 -15% - +48% -5.95ms - +22.06ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	511.63ms - 521.47ms	-	unsure 🔍 -2% - +0% -11.41ms - +2.09ms	unsure 🔍 -2% - +1% -8.50ms - +4.33ms
tip-of-tree tip-of-tree	516.59ms - 525.82ms	unsure 🔍 -0% - +2% -2.09ms - +11.41ms	-	unsure 🔍 -1% - +2% -3.61ms - +8.76ms
previous-release previous-release	514.52ms - 522.75ms	unsure 🔍 -1% - +2% -4.33ms - +8.50ms	unsure 🔍 -2% - +1% -8.76ms - +3.61ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	523.29ms - 529.70ms	-	unsure 🔍 -1% - +1% -4.60ms - +4.43ms	unsure 🔍 -1% - +1% -3.54ms - +5.05ms
tip-of-tree tip-of-tree	523.39ms - 529.77ms	unsure 🔍 -1% - +1% -4.43ms - +4.60ms	-	unsure 🔍 -1% - +1% -3.44ms - +5.13ms
previous-release previous-release	522.88ms - 528.60ms	unsure 🔍 -1% - +1% -5.05ms - +3.54ms	unsure 🔍 -1% - +1% -5.13ms - +3.44ms	-

_{tachometer-reporter-action v2 for Benchmarks}

justinfagnani · 2025-04-14T18:51:24Z

packages/lit-html/src/test/directive-helpers_test.ts

-        const childPart2 = insertPart(part, undefined);
-        const childPart1 = insertPart(part, undefined, childPart2);
+        const childPart2 = insertPart(part);
+        const childPart1 = insertPart(part, childPart2);


Previously the test mixed up the parameter for the refPart and the part to be inserted. Passing the third parameter meant that the second call wasn't making a new part.

github-actions · 2025-04-14T18:52:21Z

The size of lit-html.js and lit-core.min.js are as expected.

rictic · 2025-04-15T19:24:52Z

This passes all of Google's internal tests!

justinfagnani · 2025-04-15T21:03:53Z

Thanks for running the google3 tests, @rictic!

jimfranke · 2025-06-13T07:02:04Z

@justinfagnani It looks like this change causes some unforseen behaviour. When using the CustomElement below to reshuffle a list of items with varying length via the repeat directive by pressing the button, I see markers  being added and never being removed within the ul element; resulting in hundreds of dangling markers. When reverting this PR everything seems to behave as expected.

Could you please verify? Thanks!

<!doctype html>
<html lang="en">

<head>
  <meta charset="utf-8" />
  <title>App</title>

  <script type="module">
    //*! Import 'html' and 'render' versions from this PR !*/
    
    // Import the latest 'repeat' directive
    import { repeat } from 'https://cdn.jsdelivr.net/npm/[email protected]/directives/repeat.js';

    const fruits = [{
        name: 'apple',
        text: '🍎'
      },
      {
        name: 'banana',
        text: '🍌'
      },
      {
        name: 'grapes',
        text: '🍇'
      },
      {
        name: 'pineapple',
        text: '🍍'
      },
      {
        name: 'peach',
        text: '🍑'
      },
      {
        name: 'cherry',
        text: '🍒'
      },
      {
        name: 'greenapple',
        text: '🍏'
      },
      {
        name: 'melon',
        text: '🍉'
      },
      {
        name: 'strawberry',
        text: '🍓'
      },
    ];

    customElements.define(
      'x-app',
      class App extends HTMLElement {
        #fruits = [...fruits];

        connectedCallback() {
          this.render();
        }

        shuffleFruits = () => {
          const size = Math.floor(Math.random() * fruits.length) + 1;

          this.#fruits = fruits.sort(() => Math.random() - 0.5).slice(-size);
          console.log(this.#fruits.concat());
          this.render();
        };

        render() {
          render(
            html`
          <main>
            <button @click=${this.shuffleFruits}>Shuffle Fruits</button>

            <ul>
              ${repeat(
                this.#fruits,
                fruit => fruit.name,
                fruit =>
                  html`<li>
                    <label>
                      <input name=${fruit.name} type="checkbox" />
                      ${fruit.name} - ${fruit.text}
                    </label>
                  </li>`,
              )}
            </ul>
          </main>
        `,
            this,
          );
        }
      },
    );
  </script>
</head>

<body>
  <x-app></x-app>
</body>

</html>

justinfagnani · 2025-06-13T08:29:08Z

@jimfranke since this PR is merged, can you open a new issue so we can be sure to track it?

justinfagnani · 2025-06-13T08:33:14Z

I do think I see the problem here. We need some more tests for repeat and directive helpers.

jimfranke · 2025-06-13T09:23:09Z

@jimfranke since this PR is merged, can you open a new issue so we can be sure to track it?

Thanks, sure! #5010

[lit-html] Remove some redundant code from removePart()

fc83c46

Also fixes a directive-helpers test that was incorrectly constructing its test case.

justinfagnani requested a review from sorvell April 14, 2025 18:50

justinfagnani requested a review from kevinpschaaf as a code owner April 14, 2025 18:50

justinfagnani commented Apr 14, 2025

View reviewed changes

justinfagnani added 2 commits April 14, 2025 11:53

Update expected library sizes

67ca4d2

Remove private API use in tests

bc5514d

justinfagnani requested a review from rictic April 15, 2025 21:04

rictic approved these changes Apr 15, 2025

View reviewed changes

justinfagnani merged commit 43a3f4d into main Apr 15, 2025
9 checks passed

justinfagnani deleted the remove-part-golf branch April 15, 2025 21:57

jimfranke mentioned this pull request Jun 13, 2025

removePart() and repeat directive causes dangling comment markers #5010

Open

lit-robot mentioned this pull request Jul 11, 2025

Version Packages #5019

Merged

[lit-html] Remove some redundant code from removePart() #4975

[lit-html] Remove some redundant code from removePart() #4975

Uh oh!

Conversation

justinfagnani commented Apr 14, 2025

Uh oh!

changeset-bot bot commented Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot commented Apr 14, 2025 • 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

update-reflect

Uh oh!

justinfagnani Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rictic commented Apr 15, 2025

Uh oh!

justinfagnani commented Apr 15, 2025

Uh oh!

Uh oh!

jimfranke commented Jun 13, 2025

Uh oh!

justinfagnani commented Jun 13, 2025

Uh oh!

justinfagnani commented Jun 13, 2025

Uh oh!

jimfranke commented Jun 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

changeset-bot bot commented Apr 14, 2025 •

edited

Loading

github-actions bot commented Apr 14, 2025 •

edited

Loading

github-actions bot commented Apr 14, 2025 •

edited

Loading