Don't throw the shadowed properties dev-mode error when a property is intentionally not reactive. #2160

bicknellr · 2021-09-14T23:02:38Z

This PR prevents the dev-mode error about shadowed properties from being thrown if the property is configured not to have a generated descriptor. This includes setting noAccessor in the properties block / @property decorator and returning undefined from getPropertyDescriptor.

This PR also adds undefined as a possible return type for getPropertyDescriptor, which is documented as if it should be able to do this, but currently isn't allowed due to the inferred return type. This change required adding a few ! assertions in some existing tests.

…be reactive.

github-actions · 2021-09-14T23:03:02Z

📊 Tachometer Benchmark Results

Summary

⏳ Benchmarks are currently running. Results below are out of date.

nop-update

⏱ lit-html-kitchen-sink: unsure 🔍 -1% - +1% (-0.40ms - +0.32ms)
^{this-change vs tip-of-tree}

render

⏱ lit-element-list: unsure 🔍 -1% - +0% (-0.80ms - +0.25ms)
^{this-change vs tip-of-tree}
⏱ lit-html-kitchen-sink: faster ✔ 0% - 1% (0.03ms - 0.33ms)
^{this-change vs tip-of-tree}
⏱ lit-html-repeat: unsure 🔍 -2% - +2% (-0.21ms - +0.19ms)
^{this-change vs tip-of-tree}
⏱ lit-html-template-heavy: unsure 🔍 -0% - +2% (-0.27ms - +1.20ms)
^{this-change vs tip-of-tree}
⏱ reactive-element-list: unsure 🔍 -1% - +2% (-0.45ms - +1.12ms)
^{this-change vs tip-of-tree}

update

⏱ lit-element-list: unsure 🔍 -2% - +1% (-16.81ms - +7.66ms)
^{this-change vs tip-of-tree}
⏱ lit-html-kitchen-sink: unsure 🔍 -2% - +2% (-1.76ms - +1.65ms)
^{this-change vs tip-of-tree}
⏱ lit-html-repeat: unsure 🔍 -2% - +1% (-5.44ms - +4.80ms)
^{this-change vs tip-of-tree}
⏱ lit-html-template-heavy: unsure 🔍 -1% - +2% (-1.52ms - +2.18ms)
^{this-change vs tip-of-tree}
⏱ reactive-element-list: unsure 🔍 -1% - +2% (-7.58ms - +14.93ms)
^{this-change vs tip-of-tree}

update-reflect

⏱ lit-element-list: unsure 🔍 -2% - +1% (-16.58ms - +9.11ms)
^{this-change vs tip-of-tree}
⏱ reactive-element-list: unsure 🔍 -2% - +1% (-15.32ms - +11.15ms)
^{this-change vs tip-of-tree}

Results

⏳ Benchmarks are currently running. Results below are out of date.

⏱ lit-element-list

Browser: chrome-headless 93.0.4577.82
Sample size: 50
Built by: Benchmarks #1965
Commit: 32c0e4b

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	86.80ms - 87.56ms	-	unsure 🔍 -1% - +0% -0.80ms - +0.25ms	faster ✔ 19% - 20% 20.28ms - 21.36ms
tip-of-tree tip-of-tree	87.10ms - 87.82ms	unsure 🔍 -0% - +1% -0.25ms - +0.80ms	-	faster ✔ 19% - 19% 20.02ms - 21.07ms
previous-release previous-release	107.62ms - 108.38ms	slower ❌ 23% - 25% 20.28ms - 21.36ms	slower ❌ 23% - 24% 20.02ms - 21.07ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	712.68ms - 729.57ms	-	unsure 🔍 -2% - +1% -16.81ms - +7.66ms	faster ✔ 7% - 10% 57.93ms - 81.94ms
tip-of-tree tip-of-tree	716.85ms - 734.55ms	unsure 🔍 -1% - +2% -7.66ms - +16.81ms	-	faster ✔ 7% - 10% 53.07ms - 77.66ms
previous-release previous-release	782.53ms - 799.60ms	slower ❌ 8% - 11% 57.93ms - 81.94ms	slower ❌ 7% - 11% 53.07ms - 77.66ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	760.30ms - 778.18ms	-	unsure 🔍 -2% - +1% -16.58ms - +9.11ms	faster ✔ 4% - 7% 28.84ms - 53.73ms
tip-of-tree tip-of-tree	763.76ms - 782.20ms	unsure 🔍 -1% - +2% -9.11ms - +16.58ms	-	faster ✔ 3% - 6% 24.90ms - 50.20ms
previous-release previous-release	801.86ms - 819.19ms	slower ❌ 4% - 7% 28.84ms - 53.73ms	slower ❌ 3% - 7% 24.90ms - 50.20ms	-

⏱ lit-html-kitchen-sink

Browser: chrome-headless 93.0.4577.82
Sample size: 30
Built by: Benchmarks #1965
Commit: 32c0e4b

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	35.59ms - 35.78ms	-	faster ✔ 0% - 1% 0.03ms - 0.33ms	faster ✔ 16% - 17% 6.55ms - 7.34ms
tip-of-tree tip-of-tree	35.75ms - 35.97ms	slower ❌ 0% - 1% 0.03ms - 0.33ms	-	faster ✔ 15% - 17% 6.36ms - 7.16ms
previous-release previous-release	42.24ms - 43.01ms	slower ❌ 18% - 21% 6.55ms - 7.34ms	slower ❌ 18% - 20% 6.36ms - 7.16ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	86.65ms - 89.55ms	-	unsure 🔍 -2% - +2% -1.76ms - +1.65ms	faster ✔ 4% - 8% 3.38ms - 7.19ms
tip-of-tree tip-of-tree	87.25ms - 89.05ms	unsure 🔍 -2% - +2% -1.65ms - +1.76ms	-	faster ✔ 4% - 7% 3.71ms - 6.75ms
previous-release previous-release	92.15ms - 94.61ms	slower ❌ 4% - 8% 3.38ms - 7.19ms	slower ❌ 4% - 8% 3.71ms - 6.75ms	-

nop-update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	29.41ms - 29.97ms	-	unsure 🔍 -1% - +1% -0.40ms - +0.32ms	unsure 🔍 -3% - +0% -0.93ms - +0.15ms
tip-of-tree tip-of-tree	29.50ms - 29.95ms	unsure 🔍 -1% - +1% -0.32ms - +0.40ms	-	unsure 🔍 -3% - +1% -0.87ms - +0.16ms
previous-release previous-release	29.61ms - 30.54ms	unsure 🔍 -1% - +3% -0.15ms - +0.93ms	unsure 🔍 -1% - +3% -0.16ms - +0.87ms	-

⏱ lit-html-repeat

Browser: chrome-headless 93.0.4577.82
Sample size: 30
Built by: Benchmarks #1965
Commit: 32c0e4b

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	12.64ms - 12.91ms	-	unsure 🔍 -2% - +2% -0.21ms - +0.19ms	faster ✔ 6% - 8% 0.81ms - 1.10ms
tip-of-tree tip-of-tree	12.64ms - 12.93ms	unsure 🔍 -2% - +2% -0.19ms - +0.21ms	-	faster ✔ 6% - 8% 0.79ms - 1.11ms
previous-release previous-release	13.68ms - 13.78ms	slower ❌ 6% - 9% 0.81ms - 1.10ms	slower ❌ 6% - 9% 0.79ms - 1.11ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	358.06ms - 363.36ms	-	unsure 🔍 -2% - +1% -5.44ms - +4.80ms	faster ✔ 28% - 30% 143.03ms - 153.45ms
tip-of-tree tip-of-tree	356.65ms - 365.40ms	unsure 🔍 -1% - +2% -4.80ms - +5.44ms	-	faster ✔ 28% - 30% 141.65ms - 154.19ms
previous-release previous-release	504.46ms - 513.44ms	slower ❌ 39% - 43% 143.03ms - 153.45ms	slower ❌ 39% - 43% 141.65ms - 154.19ms	-

⏱ lit-html-template-heavy

Browser: chrome-headless 93.0.4577.82
Sample size: 50
Built by: Benchmarks #1965
Commit: 32c0e4b

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	58.20ms - 59.46ms	-	unsure 🔍 -0% - +2% -0.27ms - +1.20ms	faster ✔ 16% - 18% 11.63ms - 13.08ms
tip-of-tree tip-of-tree	57.98ms - 58.74ms	unsure 🔍 -2% - +0% -1.20ms - +0.27ms	-	faster ✔ 17% - 19% 12.30ms - 13.34ms
previous-release previous-release	70.82ms - 71.54ms	slower ❌ 20% - 22% 11.63ms - 13.08ms	slower ❌ 21% - 23% 12.30ms - 13.34ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	119.85ms - 122.76ms	-	unsure 🔍 -1% - +2% -1.52ms - +2.18ms	faster ✔ 11% - 14% 15.30ms - 19.19ms
tip-of-tree tip-of-tree	119.84ms - 122.11ms	unsure 🔍 -2% - +1% -2.18ms - +1.52ms	-	faster ✔ 12% - 14% 15.85ms - 19.29ms
previous-release previous-release	137.26ms - 139.84ms	slower ❌ 12% - 16% 15.30ms - 19.19ms	slower ❌ 13% - 16% 15.85ms - 19.29ms	-

⏱ reactive-element-list

Browser: chrome-headless 93.0.4577.82
Sample size: 50
Built by: Benchmarks #1965
Commit: 32c0e4b

render

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	58.95ms - 59.97ms	-	unsure 🔍 -1% - +2% -0.45ms - +1.12ms	unsure 🔍 -1% - +1% -0.68ms - +0.79ms
tip-of-tree tip-of-tree	58.53ms - 59.72ms	unsure 🔍 -2% - +1% -1.12ms - +0.45ms	-	unsure 🔍 -2% - +1% -1.07ms - +0.51ms
previous-release previous-release	58.88ms - 59.93ms	unsure 🔍 -1% - +1% -0.79ms - +0.68ms	unsure 🔍 -1% - +2% -0.51ms - +1.07ms	-

update

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	712.92ms - 728.88ms	-	unsure 🔍 -1% - +2% -7.58ms - +14.93ms	unsure 🔍 -1% - +2% -5.99ms - +14.48ms
tip-of-tree tip-of-tree	709.29ms - 725.16ms	unsure 🔍 -2% - +1% -14.93ms - +7.58ms	-	unsure 🔍 -1% - +2% -9.63ms - +10.77ms
previous-release previous-release	710.24ms - 723.06ms	unsure 🔍 -2% - +1% -14.48ms - +5.99ms	unsure 🔍 -2% - +1% -10.77ms - +9.63ms	-

update-reflect

Version	Avg time	vs this-change	vs tip-of-tree tip-of-tree	vs previous-release previous-release
this-change	791.20ms - 809.35ms	-	unsure 🔍 -2% - +1% -15.32ms - +11.15ms	unsure 🔍 -1% - +2% -11.96ms - +13.30ms
tip-of-tree tip-of-tree	792.72ms - 812.00ms	unsure 🔍 -1% - +2% -11.15ms - +15.32ms	-	unsure 🔍 -1% - +2% -10.28ms - +15.79ms
previous-release previous-release	790.82ms - 808.38ms	unsure 🔍 -2% - +1% -13.30ms - +11.96ms	unsure 🔍 -2% - +1% -15.79ms - +10.28ms	-

_{tachometer-reporter-action v2 for Benchmarks}

sorvell · 2021-09-17T01:38:01Z

packages/reactive-element/src/test/reactive-element_test.ts

@@ -3075,4 +3075,95 @@ suite('ReactiveElement', () => {
      assert.equal(el2.attrValue, 'custom');
    });
  });
+
+  if (DEV_MODE) {


This should be moved to reactive-element_dev_mode_test

Ok, I moved these into that file.

changeset-bot · 2021-09-17T17:25:12Z

🦋 Changeset detected

Latest commit: ad9043c

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

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

sorvell · 2021-09-17T22:12:20Z

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

@@ -1140,7 +1153,13 @@ export abstract class ReactiveElement
        const shadowedProperties: string[] = [];
        (this.constructor as typeof ReactiveElement).elementProperties.forEach(
          (_v, p) => {
-            if (this.hasOwnProperty(p) && !this.__instanceProperties?.has(p)) {
+            if (


Rather than loop over elementProperties, this can just loop over prototypeToReactivePropertyNames.get(this.constructor.prototype), right?

Yeah, I think you're right: in createProperty, the elementProperties entry is always set before the prototypeToReactivePropertyNames entry might be set, so prototypeToReactivePropertyNames should always be the subset we want here. Updated.

…`elementProperties`.

…heck

sorvell · 2021-10-12T17:06:22Z

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

+          const reactivePropertyNames =
+            prototypeToReactivePropertyNames.get(this.prototype) ?? new Set();
+          reactivePropertyNames.add(name);
+          prototypeToReactivePropertyNames.set(


I don't think this will catch the case of shadowing a property defined in the superclass. We can probably just copy the superclass set of properties when the set is created.

e.g. something like new Set(prototypeToReactivePropertyNames.get(this.prototype.prototype))

We should add a test for that as well.

I've added a test for the case where a property defined by some ancestor class would be shadowed by a class field. Also, I had to move the initialization of this set into finalize so that the set will always exist, even if that class doesn't define any properties itself.

There's one thing I was confused about when moving the initialization: why is it safe to assume that the parent class is something with a finalize function? Wouldn't it eventually try to call finalize on HTMLElement?

It seems like I need to update this initialization to handle this case, but the surrounding code doesn't seem to need to do that. Pretty sure that it's currently lucking out on my probably-incorrect use of ! in that new Set(undefined) happens to produce an empty set.

…heck

…tor class.

…m its parent's set.

sorvell · 2021-10-12T22:31:23Z

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

@@ -644,6 +652,17 @@ export abstract class ReactiveElement
    this.elementProperties = new Map(superCtor.elementProperties);
    // initialize Map populated in observedAttributes
    this.__attributeToPropertyMap = new Map();
+    if (DEV_MODE) {


I'd rather this be in createProperty just to keep the DEV_MODE code less spread out.

I moved this here to avoid walking the prototype chain because createProperty isn't guaranteed to be called at least once for each class. Is walking it preferable to splitting these?

Also, I'm not sure when that would imply combining the list for classes where ancestors define the properties and some tail portion of the hierarchy prevents those properties from being reactive without themselves defining any properties. (The throws when reactive properties defined by an ancestor class are shadowed by class fields test is the smallest example of this.)

Good point. What about storing the set on the prototype rather than looking it up in the map. That way you'll get it on whatever superclass has it?

Ok, I moved the set initialization back into createProperty and make the class create a new one if it doesn't have its own.

…on back to `createProperty`.

bicknellr added 4 commits September 14, 2021 14:29

Only warn about shadowed properties if the property is configured to …

8442e71

…be reactive.

Allow getPropertyDescriptor to return undefined.

674bcd6

Add tests for dev-mode warnings about shadowed properties.

b29ca6d

Add a changeset.

0bbc053

google-cla bot added the cla: yes label Sep 14, 2021

bicknellr marked this pull request as ready for review September 14, 2021 23:21

bicknellr requested a review from justinfagnani as a code owner September 14, 2021 23:21

bicknellr requested a review from sorvell September 14, 2021 23:21

sorvell requested changes Sep 17, 2021

View reviewed changes

Move dev-mode tests into 'reactive-element_dev_mode_test.ts'.

533fc2e

bicknellr requested a review from sorvell September 17, 2021 17:32

sorvell requested changes Sep 17, 2021

View reviewed changes

Iterate over reactivePropertyNames directly, rather than filtering …

4f2e27f

…`elementProperties`.

bicknellr requested a review from sorvell September 17, 2021 22:43

kevinpschaaf added this to the Lit RC.5 milestone Sep 20, 2021

justinfagnani modified the milestones: Lit 2.0, Lit 2.1 Sep 21, 2021

bicknellr added 2 commits September 23, 2021 17:20

Merge remote-tracking branch 'origin/main' into shadowed-properties-c…

bafb61b

…heck

Merge remote-tracking branch 'origin/main' into shadowed-properties-c…

91ce5fa

…heck

sorvell requested changes Oct 12, 2021

View reviewed changes

bicknellr added 4 commits October 12, 2021 12:05

Merge remote-tracking branch 'origin/main' into shadowed-properties-c…

5043033

…heck

Test that the error is also thrown for properties defined in an ances…

72c65fb

…tor class.

Update some test descriptions and other names for clarity.

3ed0e40

Initialize each class' reactive property name set with the values fro…

ee101d0

…m its parent's set.

bicknellr requested a review from sorvell October 12, 2021 22:16

sorvell reviewed Oct 12, 2021

View reviewed changes

Store the set of properties on the class itself and move initializati…

ad9043c

…on back to `createProperty`.

bicknellr requested a review from sorvell October 14, 2021 19:32

sorvell approved these changes Oct 14, 2021

View reviewed changes

bicknellr merged commit 90a8c12 into main Oct 14, 2021

bicknellr deleted the shadowed-properties-check branch October 14, 2021 22:33

github-actions bot mentioned this pull request Oct 18, 2021

Version Packages abdonrd/lit#3

Closed

github-actions bot mentioned this pull request Nov 9, 2021

Version Packages #2287

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't throw the shadowed properties dev-mode error when a property is intentionally not reactive. #2160

Don't throw the shadowed properties dev-mode error when a property is intentionally not reactive. #2160

bicknellr commented Sep 14, 2021

github-actions bot commented Sep 14, 2021 •

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

sorvell Sep 17, 2021

bicknellr Sep 17, 2021

changeset-bot bot commented Sep 17, 2021 •

edited

sorvell Sep 17, 2021

bicknellr Sep 17, 2021

sorvell Oct 12, 2021

bicknellr Oct 12, 2021

bicknellr Oct 12, 2021 •

edited

sorvell Oct 12, 2021

bicknellr Oct 12, 2021

bicknellr Oct 12, 2021 •

edited

sorvell Oct 13, 2021

bicknellr Oct 14, 2021

Don't throw the shadowed properties dev-mode error when a property is intentionally not reactive. #2160

Don't throw the shadowed properties dev-mode error when a property is intentionally not reactive. #2160

Conversation

bicknellr commented Sep 14, 2021

github-actions bot commented Sep 14, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changeset-bot bot commented Sep 17, 2021 • edited

🦋 Changeset detected

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bicknellr Oct 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bicknellr Oct 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 14, 2021 •

edited

changeset-bot bot commented Sep 17, 2021 •

edited

bicknellr Oct 12, 2021 •

edited

bicknellr Oct 12, 2021 •

edited