[lit-html] Add DEV_MODE error if duplicate attribute bindings are encountered

[AndrewJakubowicz]

Fixes: #4194

Context

A lot of the context is in the issue, but in short, if an attribute binding is duplicated, such as in the following example:

<textarea 
      ?disabled=${this.disabled} 
      ?disabled=${this.disabled} 
      placeholder=${this.placeholder}
></textarea>

The result is that Lit parts get out of sync with the values (because attributes are de-duped by the browser).

Fix

I've implemented this as a DEV_MODE error, but I'm open to making it a DEV_MODE warning.

The following template: <input ?disabled=${true} ?disabled=${false}>, will throw:

Detected duplicate attribute bindings of attribute: '?disabled', in the template: `<input ?disabled=${...} ?disabled=${...}>`

[changeset-bot]

🦋 Changeset detected

Latest commit: d3d93a7

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

This PR includes changesets to release 2 packages

Name	Type
lit-html	Patch
lit	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]

📊 Tachometer Benchmark Results

Summary

nop-update

• this-change, tip-of-tree, previous-release: unsure 🔍 -6% - +2% (-0.67ms - +0.27ms)
  ^{this-change vs tip-of-tree}

render

• this-change: 48.18ms - 50.16ms
• this-change, tip-of-tree, previous-release: unsure 🔍 -6% - +0% (-1.16ms - +0.05ms)
  ^{this-change vs tip-of-tree}
• this-change, tip-of-tree, previous-release: unsure 🔍 -3% - +1% (-1.07ms - +0.51ms)
  ^{this-change vs tip-of-tree}
• this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +2% (-0.52ms - +0.78ms)
  ^{this-change vs tip-of-tree}

update

• this-change: 512.63ms - 517.43ms
• this-change, tip-of-tree, previous-release: faster ✔ 0% - 11% (0.06ms - 4.39ms)
  ^{this-change vs tip-of-tree}
• this-change, tip-of-tree, previous-release: unsure 🔍 -2% - +1% (-1.77ms - +0.77ms)
  ^{this-change vs tip-of-tree}
• this-change, tip-of-tree, previous-release: unsure 🔍 -0% - +1% (-1.53ms - +7.95ms)
  ^{this-change vs tip-of-tree}

update-reflect

• this-change: 517.08ms - 523.98ms
• this-change, tip-of-tree, previous-release: unsure 🔍 -1% - +1% (-5.25ms - +4.14ms)
  ^{this-change vs tip-of-tree}

Results

this-change

• Browser: chrome-headless 120.0.6099.224
• Sample size: 50
• Built by: Benchmarks #6064

• Commit: d3d93a7

render

Version	Avg time	vs
	48.18ms - 50.16ms	-

update

Version	Avg time	vs
	512.63ms - 517.43ms	-

update-reflect

Version	Avg time	vs
	517.08ms - 523.98ms	-

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

• Browser: chrome-headless 120.0.6099.224
• Sample size: 30
• Built by: Benchmarks #6064

• Commit: d3d93a7

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	17.99ms - 18.72ms	-	unsure 🔍 -6% - +0% -1.16ms - +0.05ms	unsure 🔍 -5% - +1% -1.04ms - +0.17ms
tip-of-tree tip-of-tree	18.43ms - 19.39ms	unsure 🔍 -0% - +6% -0.05ms - +1.16ms	-	unsure 🔍 -3% - +4% -0.56ms - +0.80ms
previous-release previous-release	18.31ms - 19.27ms	unsure 🔍 -1% - +6% -0.17ms - +1.04ms	unsure 🔍 -4% - +3% -0.80ms - +0.56ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	37.37ms - 39.71ms	-	faster ✔ 0% - 11% 0.06ms - 4.39ms	unsure 🔍 -10% - +0% -3.94ms - +0.11ms
tip-of-tree tip-of-tree	38.95ms - 42.59ms	slower ❌ 0% - 11% 0.06ms - 4.39ms	-	unsure 🔍 -5% - +7% -2.15ms - +2.77ms
previous-release previous-release	38.81ms - 42.11ms	unsure 🔍 -0% - +10% -0.11ms - +3.94ms	unsure 🔍 -7% - +5% -2.77ms - +2.15ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	10.81ms - 11.43ms	-	unsure 🔍 -6% - +2% -0.67ms - +0.27ms	unsure 🔍 -6% - +2% -0.63ms - +0.23ms
tip-of-tree tip-of-tree	10.96ms - 11.68ms	unsure 🔍 -2% - +6% -0.27ms - +0.67ms	-	unsure 🔍 -4% - +4% -0.46ms - +0.47ms
previous-release previous-release	11.01ms - 11.62ms	unsure 🔍 -2% - +6% -0.23ms - +0.63ms	unsure 🔍 -4% - +4% -0.47ms - +0.46ms	-

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

• Browser: chrome-headless 120.0.6099.224
• Sample size: 50
• Built by: Benchmarks #6064

• Commit: d3d93a7

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	33.38ms - 34.44ms	-	unsure 🔍 -3% - +1% -1.07ms - +0.51ms	unsure 🔍 -2% - +3% -0.65ms - +0.85ms
tip-of-tree tip-of-tree	33.60ms - 34.77ms	unsure 🔍 -2% - +3% -0.51ms - +1.07ms	-	unsure 🔍 -1% - +3% -0.42ms - +1.17ms
previous-release previous-release	33.27ms - 34.35ms	unsure 🔍 -3% - +2% -0.85ms - +0.65ms	unsure 🔍 -3% - +1% -1.17ms - +0.42ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	69.93ms - 71.44ms	-	unsure 🔍 -2% - +1% -1.77ms - +0.77ms	unsure 🔍 -2% - +1% -1.34ms - +0.94ms
tip-of-tree tip-of-tree	70.16ms - 72.21ms	unsure 🔍 -1% - +3% -0.77ms - +1.77ms	-	unsure 🔍 -1% - +2% -1.03ms - +1.63ms
previous-release previous-release	70.03ms - 71.74ms	unsure 🔍 -1% - +2% -0.94ms - +1.34ms	unsure 🔍 -2% - +1% -1.63ms - +1.03ms	-

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

• Browser: chrome-headless 120.0.6099.224
• Sample size: 50
• Built by: Benchmarks #6064

• Commit: d3d93a7

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	33.53ms - 34.48ms	-	unsure 🔍 -2% - +2% -0.52ms - +0.78ms	unsure 🔍 -1% - +3% -0.36ms - +0.88ms
tip-of-tree tip-of-tree	33.43ms - 34.31ms	unsure 🔍 -2% - +2% -0.78ms - +0.52ms	-	unsure 🔍 -1% - +2% -0.47ms - +0.72ms
previous-release previous-release	33.35ms - 34.14ms	unsure 🔍 -3% - +1% -0.88ms - +0.36ms	unsure 🔍 -2% - +1% -0.72ms - +0.47ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	532.06ms - 539.05ms	-	unsure 🔍 -0% - +1% -1.53ms - +7.95ms	unsure 🔍 -1% - +1% -4.86ms - +5.22ms
tip-of-tree tip-of-tree	529.14ms - 535.55ms	unsure 🔍 -1% - +0% -7.95ms - +1.53ms	-	unsure 🔍 -1% - +0% -7.87ms - +1.81ms
previous-release previous-release	531.75ms - 539.00ms	unsure 🔍 -1% - +1% -5.22ms - +4.86ms	unsure 🔍 -0% - +1% -1.81ms - +7.87ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	537.53ms - 544.40ms	-	unsure 🔍 -1% - +1% -5.25ms - +4.14ms	unsure 🔍 -1% - +1% -6.24ms - +3.35ms
tip-of-tree tip-of-tree	538.32ms - 544.72ms	unsure 🔍 -1% - +1% -4.14ms - +5.25ms	-	unsure 🔍 -1% - +1% -5.52ms - +3.75ms
previous-release previous-release	539.06ms - 545.76ms	unsure 🔍 -1% - +1% -3.35ms - +6.24ms	unsure 🔍 -1% - +1% -3.75ms - +5.52ms	-

_{tachometer-reporter-action v2 for Benchmarks}

[github-actions]

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

[justinfagnani]

lgtm!!

[justinfagnani]

I think an error is correct here. The template will throw at rendering time anyway.

[AndrewJakubowicz]

Note: Converted to draft because this collapses duplicate attributes across individual tags which is invalid behavior.

Added a test case: <input ?disabled=${...}><input ?disabled=${...}>. This should not error (and it does with the current naive implementation).

[AndrewJakubowicz]

I fixed the implementation to be correct. Throw if there are attribute names that were not considered for template preparation (detects attribute mismatch).

Adding the attribute that caused the attribute mismatch is going to be tricky as duplicate attribute names can exist across multiple tags. I think this implementation is strictly an improvement, and we provide the template in the error that caused the issue. This will reduce debugging time for user's who encounter this error.

[augustjk]

Ooohhh very clever to check that we've consumed everything from attrNames!