[reactive-element] Only use symbol keys for properties added as private storage. #3693

bicknellr · 2023-02-24T00:29:07Z

changeset-bot · 2023-02-24T00:29:09Z

🦋 Changeset detected

Latest commit: 4f9ce3c

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

This PR includes changesets to release 29 packages

Name	Type
lit-html	Major
lit	Major
lit-element	Major
@lit/reactive-element	Major
@lit/lit-starter-js	Major
@lit/lit-starter-ts	Major
@lit-internal/benchmarks	Patch
@lit-labs/ssr-client	Patch
@lit-labs/ssr	Patch
@lit/localize-tools	Patch
@lit/localize	Patch
@lit-labs/context	Patch
@lit-labs/eleventy-plugin-lit	Patch
@lit-labs/motion	Patch
@lit-labs/router	Patch
@lit-labs/scoped-registry-mixin	Patch
@lit-labs/ssr-react	Patch
@lit-labs/testing	Patch
@lit-labs/virtualizer	Patch
@lit-internal/test-element-a	Patch
@lit-internal/localize-examples-runtime-js	Patch
@lit-internal/localize-examples-runtime-ts	Patch
@lit-internal/localize-examples-transform-js	Patch
@lit-internal/localize-examples-transform-ts	Patch
@lit-labs/observers	Patch
@lit-labs/task	Patch
@lit-labs/cli-localize	Patch
@lit-labs/cli	Patch
@lit-labs/nextjs	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 · 2023-02-24T00:29:37Z

📊 Tachometer Benchmark Results

Summary

nop-update

lit-html-kitchen-sink: unsure 🔍 -3% - +10% (-0.39ms - +1.45ms)
^{this-change vs tip-of-tree}

render

lit-element-list: 71.30ms - 74.78ms
lit-html-kitchen-sink: unsure 🔍 -2% - +11% (-0.55ms - +3.35ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -6% - +3% (-0.66ms - +0.32ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: slower ❌ 1% - 7% (0.39ms - 3.77ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -2% - +4% (-0.76ms - +1.93ms)
^{this-change vs tip-of-tree}

update

lit-element-list: 657.27ms - 661.34ms
lit-html-kitchen-sink: unsure 🔍 -8% - +2% (-5.69ms - +1.81ms)
^{this-change vs tip-of-tree}
lit-html-repeat: unsure 🔍 -13% - +10% (-36.27ms - +27.37ms)
^{this-change vs tip-of-tree}
lit-html-template-heavy: unsure 🔍 -1% - +2% (-0.98ms - +1.74ms)
^{this-change vs tip-of-tree}
reactive-element-list: unsure 🔍 -0% - +1% (-2.46ms - +4.25ms)
^{this-change vs tip-of-tree}

update-reflect

lit-element-list: 641.96ms - 647.10ms
reactive-element-list: unsure 🔍 -0% - +0% (-2.92ms - +2.44ms)
^{this-change vs tip-of-tree}

Results

lit-element-list

Browser: chrome-headless 112.0.5615.49
Sample size: 50
Built by: Benchmarks #4646
Commit: 6824280

render

Version	Avg time	vs
	71.30ms - 74.78ms	-

update

Version	Avg time	vs
	657.27ms - 661.34ms	-

update-reflect

Version	Avg time	vs
	641.96ms - 647.10ms	-

lit-html-kitchen-sink

Browser: chrome-headless 112.0.5615.49
Sample size: 30
Built by: Benchmarks #4646
Commit: 6824280

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	31.00ms - 33.87ms	-	unsure 🔍 -2% - +11% -0.55ms - +3.35ms	unsure 🔍 -2% - +11% -0.70ms - +3.27ms
tip-of-tree tip-of-tree	29.71ms - 32.36ms	unsure 🔍 -10% - +2% -3.35ms - +0.55ms	-	unsure 🔍 -6% - +6% -2.03ms - +1.80ms
previous-release previous-release	29.77ms - 32.53ms	unsure 🔍 -10% - +2% -3.27ms - +0.70ms	unsure 🔍 -6% - +7% -1.80ms - +2.03ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	69.28ms - 74.12ms	-	unsure 🔍 -8% - +2% -5.69ms - +1.81ms	unsure 🔍 -7% - +2% -4.94ms - +1.78ms
tip-of-tree tip-of-tree	70.77ms - 76.51ms	unsure 🔍 -3% - +8% -1.81ms - +5.69ms	-	unsure 🔍 -5% - +6% -3.33ms - +4.06ms
previous-release previous-release	70.95ms - 75.61ms	unsure 🔍 -3% - +7% -1.78ms - +4.94ms	unsure 🔍 -5% - +5% -4.06ms - +3.33ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	14.92ms - 16.54ms	-	unsure 🔍 -3% - +10% -0.39ms - +1.45ms	unsure 🔍 -10% - +9% -1.55ms - +1.39ms
tip-of-tree tip-of-tree	14.77ms - 15.63ms	unsure 🔍 -9% - +2% -1.45ms - +0.39ms	-	unsure 🔍 -12% - +4% -1.91ms - +0.69ms
previous-release previous-release	14.58ms - 17.04ms	unsure 🔍 -9% - +10% -1.39ms - +1.55ms	unsure 🔍 -5% - +13% -0.69ms - +1.91ms	-

lit-html-repeat

Browser: chrome-headless 112.0.5615.49
Sample size: 30
Built by: Benchmarks #4646
Commit: 6824280

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	10.18ms - 10.77ms	-	unsure 🔍 -6% - +3% -0.66ms - +0.32ms	unsure 🔍 -5% - +2% -0.53ms - +0.26ms
tip-of-tree tip-of-tree	10.26ms - 11.04ms	unsure 🔍 -3% - +6% -0.32ms - +0.66ms	-	unsure 🔍 -4% - +5% -0.44ms - +0.51ms
previous-release previous-release	10.35ms - 10.88ms	unsure 🔍 -2% - +5% -0.26ms - +0.53ms	unsure 🔍 -5% - +4% -0.51ms - +0.44ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	247.49ms - 287.97ms	-	unsure 🔍 -13% - +10% -36.27ms - +27.37ms	unsure 🔍 -15% - +8% -41.15ms - +23.30ms
tip-of-tree tip-of-tree	247.63ms - 296.74ms	unsure 🔍 -10% - +14% -27.37ms - +36.27ms	-	unsure 🔍 -14% - +11% -39.57ms - +30.63ms
previous-release previous-release	251.58ms - 301.74ms	unsure 🔍 -9% - +16% -23.30ms - +41.15ms	unsure 🔍 -11% - +15% -30.63ms - +39.57ms	-

lit-html-template-heavy

Browser: chrome-headless 112.0.5615.49
Sample size: 50
Built by: Benchmarks #4646
Commit: 6824280

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	53.48ms - 55.88ms	-	slower ❌ 1% - 7% 0.39ms - 3.77ms	unsure 🔍 -2% - +4% -1.08ms - +2.30ms
tip-of-tree tip-of-tree	51.41ms - 53.80ms	faster ✔ 1% - 7% 0.39ms - 3.77ms	-	unsure 🔍 -6% - +0% -3.15ms - +0.22ms
previous-release previous-release	52.88ms - 55.26ms	unsure 🔍 -4% - +2% -2.30ms - +1.08ms	unsure 🔍 -0% - +6% -0.22ms - +3.15ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	99.52ms - 101.47ms	-	unsure 🔍 -1% - +2% -0.98ms - +1.74ms	unsure 🔍 -1% - +2% -0.97ms - +1.96ms
tip-of-tree tip-of-tree	99.17ms - 101.06ms	unsure 🔍 -2% - +1% -1.74ms - +0.98ms	-	unsure 🔍 -1% - +2% -1.32ms - +1.56ms
previous-release previous-release	98.91ms - 101.09ms	unsure 🔍 -2% - +1% -1.96ms - +0.97ms	unsure 🔍 -2% - +1% -1.56ms - +1.32ms	-

reactive-element-list

Browser: chrome-headless 112.0.5615.49
Sample size: 50
Built by: Benchmarks #4646
Commit: 6824280

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	49.11ms - 51.17ms	-	unsure 🔍 -2% - +4% -0.76ms - +1.93ms	unsure 🔍 -4% - +2% -1.90ms - +1.05ms
tip-of-tree tip-of-tree	48.69ms - 50.42ms	unsure 🔍 -4% - +1% -1.93ms - +0.76ms	-	unsure 🔍 -5% - +1% -2.37ms - +0.35ms
previous-release previous-release	49.51ms - 51.62ms	unsure 🔍 -2% - +4% -1.05ms - +1.90ms	unsure 🔍 -1% - +5% -0.35ms - +2.37ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	662.22ms - 666.88ms	-	unsure 🔍 -0% - +1% -2.46ms - +4.25ms	unsure 🔍 -0% - +1% -3.22ms - +4.08ms
tip-of-tree tip-of-tree	661.23ms - 666.07ms	unsure 🔍 -1% - +0% -4.25ms - +2.46ms	-	unsure 🔍 -1% - +0% -4.17ms - +3.24ms
previous-release previous-release	661.31ms - 666.93ms	unsure 🔍 -1% - +0% -4.08ms - +3.22ms	unsure 🔍 -0% - +1% -3.24ms - +4.17ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	668.87ms - 672.12ms	-	unsure 🔍 -0% - +0% -2.92ms - +2.44ms	unsure 🔍 -0% - +0% -2.96ms - +2.86ms
tip-of-tree tip-of-tree	668.59ms - 672.87ms	unsure 🔍 -0% - +0% -2.44ms - +2.92ms	-	unsure 🔍 -0% - +1% -3.03ms - +3.42ms
previous-release previous-release	668.12ms - 672.95ms	unsure 🔍 -0% - +0% -2.86ms - +2.96ms	unsure 🔍 -1% - +0% -3.42ms - +3.03ms	-

_{tachometer-reporter-action v2 for Benchmarks}

justinfagnani · 2023-02-24T02:23:44Z

I think if we do this it'll be a breaking change, since we don't call Symbol() in the core libraries unless you're making a symbol-keyed reactive property. So this would force a polyfill where we don't now.

I'm actually ok with this as part of dropping IE11 support though, but we need to do that with a new major release in mind, and pull all being changes into Google one-by-one.

justinfagnani · 2023-04-14T21:14:59Z

@bicknellr let's do this as part of 3.0. Can you rebase onto the 3.0 branch? I could take the PR over too.

… as keys to avoid conflicts.

…a known property name.

…`__`-prefixed property.

bicknellr · 2023-04-14T22:12:34Z

SGTM, rebased.

justinfagnani · 2023-04-14T22:58:19Z

packages/reactive-element/src/decorators/query.ts

@@ -50,20 +52,18 @@ export function query(selector: string, cache?: boolean) {
        configurable: true,
      };
      if (cache) {
-        const key = typeof name === 'symbol' ? Symbol() : `__${name}`;
+        const key = DEV_MODE
+          ? Symbol(`${String(name)} (@query() cache)`)


For already Symbol-keyed properties, this would give the property a "name" of something like "Symbol(Symbol()) (@query() cache)". I think that's probably fine, and we can change if someone complains.

justinfagnani · 2023-04-14T23:02:33Z

Thanks @bicknellr !

bicknellr changed the title ~~[reactive-element] Internal storage for reactive properties should only be keyed by symbols.~~ [reactive-element] Only use symbol keys for properties added as private storage. Feb 24, 2023

augustjk mentioned this pull request Apr 13, 2023

[all] Lit 3.0 prereleases #3774

Closed

10 tasks

justinfagnani added the 3.0 label Apr 14, 2023

bicknellr added 4 commits April 14, 2023 15:05

Properties added to elements for private storage now only use symbols…

f764264

… as keys to avoid conflicts.

Give descriptions to cache property key symbols in dev mode.

98c07bf

Remove test assertions that assume reactive properties are backed by …

46848f5

…a known property name.

Test that cache property for @property no longer collides with the …

4f9ce3c

…`__`-prefixed property.

bicknellr force-pushed the symbol-backed-storage-only branch from 31ed9e9 to 4f9ce3c Compare April 14, 2023 22:10

bicknellr changed the base branch from main to 3.0 April 14, 2023 22:11

bicknellr marked this pull request as ready for review April 14, 2023 22:12

bicknellr requested a review from justinfagnani as a code owner April 14, 2023 22:12

justinfagnani approved these changes Apr 14, 2023

View reviewed changes

justinfagnani merged commit 342b75e into 3.0 Apr 14, 2023
7 checks passed

justinfagnani deleted the symbol-backed-storage-only branch April 14, 2023 23:02

justinfagnani assigned bicknellr Aug 15, 2023

rictic mentioned this pull request Oct 2, 2023

[reactive-element] Leading double underscore in a property name is ignored #3690

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[reactive-element] Only use symbol keys for properties added as private storage. #3693

[reactive-element] Only use symbol keys for properties added as private storage. #3693

bicknellr commented Feb 24, 2023 •

edited

changeset-bot bot commented Feb 24, 2023 •

edited

github-actions bot commented Feb 24, 2023 •

edited

📊 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

justinfagnani commented Feb 24, 2023

justinfagnani commented Apr 14, 2023

bicknellr commented Apr 14, 2023

justinfagnani Apr 14, 2023

justinfagnani commented Apr 14, 2023

[reactive-element] Only use symbol keys for properties added as private storage. #3693

[reactive-element] Only use symbol keys for properties added as private storage. #3693

Conversation

bicknellr commented Feb 24, 2023 • edited

changeset-bot bot commented Feb 24, 2023 • edited

🦋 Changeset detected

github-actions bot commented Feb 24, 2023 • edited

📊 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

justinfagnani commented Feb 24, 2023

justinfagnani commented Apr 14, 2023

bicknellr commented Apr 14, 2023

justinfagnani Apr 14, 2023

Choose a reason for hiding this comment

justinfagnani commented Apr 14, 2023

bicknellr commented Feb 24, 2023 •

edited

changeset-bot bot commented Feb 24, 2023 •

edited

github-actions bot commented Feb 24, 2023 •

edited