[reactive-element] Always wrap user accessors (#4146)

.changeset/hungry-cups-destroy.md

@@ -0,0 +1,7 @@
+---
+'@lit/reactive-element': major
+'lit-element': major
+'lit': major
+---
+
+Generated accessor for reactive properties now wrap user accessors and automatically call `this.requestUpdate()` in the setter. As in previous versions, users can still specify `noAccessor: true`, in which case they should call `this.requestUpdate()` themselves in the setter if they want to trigger a reactive update.

packages/reactive-element/src/legacy-decorators/property.ts

@@ -43,7 +43,16 @@ const legacyProperty = (
   proto: Object,
   name: PropertyKey
 ) => {
+  const hasOwnProperty = proto.hasOwnProperty(name);
   (proto.constructor as typeof ReactiveElement).createProperty(name, options);
+  // For accessors (which have a descriptor on the prototype) we need to
+  // return a descriptor, otherwise TypeScript overwrites the descriptor we
+  // define in createProperty() with the original descriptor. We don't do this
+  // for fields, which don't have a descriptor, because this could overwrite
+  // descriptor defined by other decorators.
+  return hasOwnProperty
+    ? Object.getOwnPropertyDescriptor(proto, name)
+    : undefined;
 };
 
 /**

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

@@ -31,7 +31,13 @@ export type {
 } from './reactive-controller.js';
 
 // TODO (justinfagnani): Add `hasOwn` here when we ship ES2022
-const {is, defineProperty, getOwnPropertyNames, getOwnPropertySymbols} = Object;
+const {
+  is,
+  defineProperty,
+  getOwnPropertyDescriptor,
+  getOwnPropertyNames,
+  getOwnPropertySymbols,
+} = Object;
 
 const NODE_MODE = false;
 
@@ -668,12 +674,7 @@ export abstract class ReactiveElement
     // is called before `finalize`, we ensure finalization has been kicked off.
     this.finalize();
     this.elementProperties.set(name, options);
-    // Do not generate an accessor if the prototype already has one, since
-    // it would be lost otherwise and that would never be the user's intention;
-    // Instead, we expect users to call `requestUpdate` themselves from
-    // user-defined accessors. Note that if the super has an accessor we will
-    // still overwrite it
-    if (!options.noAccessor && !this.prototype.hasOwnProperty(name)) {
+    if (!options.noAccessor) {
       const key = DEV_MODE
         ? // Use Symbol.for in dev mode to make it easier to maintain state
           // when doing HMR.
@@ -728,21 +729,22 @@ export abstract class ReactiveElement
     key: string | symbol,
     options: PropertyDeclaration
   ): PropertyDescriptor | undefined {
+    const {get, set} = getOwnPropertyDescriptor(this.prototype, name) ?? {
+      get(this: ReactiveElement) {
+        return this[key as keyof typeof this];
+      },
+      set(this: ReactiveElement, v: unknown) {
+        (this as unknown as Record<string | symbol, unknown>)[key] = v;
+      },
+    };
     return {
-      // eslint-disable-next-line @typescript-eslint/no-explicit-any
-      get(): any {
-        return (this as {[key: string]: unknown})[key as string];
+      get() {
+        return get!.call(this);
       },
       set(this: ReactiveElement, value: unknown) {
-        const oldValue = (this as {} as {[key: string]: unknown})[
-          name as string
-        ];
-        (this as {} as {[key: string]: unknown})[key as string] = value;
-        (this as unknown as ReactiveElement).requestUpdate(
-          name,
-          oldValue,
-          options
-        );
+        const oldValue = get!.call(this);
+        set!.call(this, value);
+        this.requestUpdate(name, oldValue, options);
       },
       configurable: true,
       enumerable: true,
@@ -837,6 +839,22 @@ export abstract class ReactiveElement
       }
     }
     this.elementStyles = this.finalizeStyles(this.styles);
+    if (DEV_MODE) {
+      if (this.hasOwnProperty('createProperty')) {
+        issueWarning(
+          'no-override-create-property',
+          'Overriding ReactiveElement.createProperty() is deprecated. ' +
+            'The override will not be called with standard decorators'
+        );
+      }
+      if (this.hasOwnProperty('getPropertyDescriptor')) {
+        issueWarning(
+          'no-override-get-property-descriptor',
+          'Overriding ReactiveElement.getPropertyDescriptor() is deprecated. ' +
+            'The override will not be called with standard decorators'
+        );
+      }
+    }
     return true;
   }
 

packages/reactive-element/src/std-decorators/property.ts

@@ -124,7 +124,9 @@ export const property = (
           // the legacy decorators, to both populate the changedProperties map
           // and to perform the initial reflection. But standard decorators
           // call init() too early and it would be an error to read the field
-          // to get the previous value at that point.
+          // to get the previous value at that point. So we pass initial=true
+          // and the initial value so requestUpdate() doesn't have to access
+          // the field.
           // We can switch to using context.addInitializer() when
           // https://github.com/tc39/proposal-decorators/issues/513 is
           // resolved and shipping in TypeScript and Babel.
@@ -139,10 +141,12 @@ export const property = (
         },
       };
     } else if (kind === 'setter') {
-      // We do not wrap the author's setter. They are expected to call
-      // requestUpdate themselves, and will possibly do so conditionally.
-      // eslint-disable-next-line @typescript-eslint/no-explicit-any
-      return undefined as any;
+      const {name} = context;
+      return function (this: C, value: V) {
+        const oldValue = this[name as keyof C];
+        (target as (value: V) => void).call(this, value);
+        this.requestUpdate(name, oldValue, options);
+      };
     }
     throw new Error(`Unsupported decorator location: ${kind}`);
   }) as PropertyDecorator;

packages/reactive-element/src/test/decorators/property_test.ts

@@ -125,36 +125,72 @@ suite('@property', () => {
 
   test('can decorate user accessor with @property', async () => {
     class E extends ReactiveElement {
+      updatedProperties?: PropertyValues<this>;
+
       _foo?: number;
-      updatedContent?: number;
 
-      @property({reflect: true, type: Number})
       get foo() {
         return this._foo as number;
       }
 
+      @property({reflect: true, type: Number})
       set foo(v: number) {
-        const old = this.foo;
         this._foo = v;
-        this.requestUpdate('foo', old);
       }
 
-      override updated() {
-        this.updatedContent = this.foo;
+      _bar?: number;
+
+      get bar() {
+        return this._bar as number;
+      }
+
+      @property({reflect: true, type: Number})
+      set bar(v: number) {
+        this._bar = v;
+      }
+
+      override updated(changedProperties: PropertyValues<this>) {
+        this.updatedProperties = changedProperties;
       }
     }
     customElements.define(generateElementName(), E);
     const el = new E();
     container.appendChild(el);
     await el.updateComplete;
+
+    // Check initial values
     assert.equal(el._foo, undefined);
-    assert.equal(el.updatedContent, undefined);
+    assert.isFalse(el.updatedProperties!.has('foo'));
     assert.isFalse(el.hasAttribute('foo'));
+    assert.equal(el._bar, undefined);
+    assert.isFalse(el.updatedProperties!.has('bar'));
+    assert.isFalse(el.hasAttribute('bar'));
+
+    // Setting values should reflect and populate changedProperties
     el.foo = 5;
+    el.bar = 6;
     await el.updateComplete;
+
     assert.equal(el._foo, 5);
-    assert.equal(el.updatedContent, 5);
+    assert.isTrue(el.updatedProperties!.has('foo'));
+    assert.equal(el.updatedProperties!.get('foo'), undefined);
     assert.equal(el.getAttribute('foo'), '5');
+    assert.equal(el._bar, 6);
+    assert.isTrue(el.updatedProperties!.has('bar'));
+    assert.equal(el.updatedProperties!.get('bar'), undefined);
+    assert.equal(el.getAttribute('bar'), '6');
+
+    // Setting values again should populate changedProperties with old values
+    el.foo = 7;
+    el.bar = 8;
+    await el.updateComplete;
+
+    assert.equal(el._foo, 7);
+    assert.equal(el.updatedProperties!.get('foo'), 5);
+    assert.equal(el.getAttribute('foo'), '7');
+    assert.equal(el._bar, 8);
+    assert.equal(el.updatedProperties!.get('bar'), 6);
+    assert.equal(el.getAttribute('bar'), '8');
   });
 
   test('can mix property options via decorator and via getter', async () => {

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

@@ -1160,9 +1160,7 @@ suite('ReactiveElement', () => {
       }
 
       set bar(value) {
-        const old = this.bar;
         this.__bar = Number(value);
-        this.requestUpdate('bar', old);
       }
 
       override updated() {
@@ -1213,9 +1211,7 @@ suite('ReactiveElement', () => {
       }
 
       set bar(value) {
-        const old = this.bar;
         this.__bar = Number(value);
-        this.requestUpdate('bar', old);
       }
     }
     customElements.define(generateElementName(), E);
@@ -1255,9 +1251,7 @@ suite('ReactiveElement', () => {
       }
 
       set foo(value) {
-        const old = this.foo;
         this.__foo = Number(value);
-        this.requestUpdate('foo', old);
       }
     }
     class F extends E {
@@ -1272,9 +1266,7 @@ suite('ReactiveElement', () => {
       }
 
       set bar(value) {
-        const old = this.foo;
         this.__bar = value;
-        this.requestUpdate('bar', old);
       }
     }
 

packages/reactive-element/src/test/std-decorators/property_test.ts

@@ -149,36 +149,48 @@ suite('@property', () => {
 
   test('can decorate user accessor with @property', async () => {
     class E extends ReactiveElement {
-      _foo?: number;
-      updatedContent?: number;
+      updatedProperties?: PropertyValues<this>;
 
+      _foo?: number;
       @property({reflect: true, type: Number})
       set foo(v: number) {
-        const old = this.foo;
         this._foo = v;
-        this.requestUpdate('foo', old);
       }
 
       get foo() {
         return this._foo as number;
       }
 
-      override updated() {
-        this.updatedContent = this.foo;
+      override updated(changedProperties: PropertyValues<this>) {
+        this.updatedProperties = changedProperties;
       }
     }
     customElements.define(generateElementName(), E);
     const el = new E();
     container.appendChild(el);
     await el.updateComplete;
+
+    // Check initial values
     assert.equal(el._foo, undefined);
-    assert.equal(el.updatedContent, undefined);
+    assert.isFalse(el.updatedProperties!.has('foo'));
     assert.isFalse(el.hasAttribute('foo'));
+
+    // Setting values should reflect and populate changedProperties
     el.foo = 5;
     await el.updateComplete;
+
     assert.equal(el._foo, 5);
-    assert.equal(el.updatedContent, 5);
+    assert.isTrue(el.updatedProperties!.has('foo'));
+    assert.equal(el.updatedProperties!.get('foo'), undefined);
     assert.equal(el.getAttribute('foo'), '5');
+
+    // Setting values again should populate changedProperties with old values
+    el.foo = 7;
+    await el.updateComplete;
+
+    assert.equal(el._foo, 7);
+    assert.equal(el.updatedProperties!.get('foo'), 5);
+    assert.equal(el.getAttribute('foo'), '7');
   });
 
   test('can mix property options via decorator and via getter', async () => {

scripts/check-size.js

@@ -9,7 +9,7 @@ import * as fs from 'fs';
 // it's likely that we'll ask you to investigate ways to reduce the size.
 //
 // In either case, update the size here and push a new commit to your PR.
-const expectedLitCoreSize = 15023;
+const expectedLitCoreSize = 15115;
 const expectedLitHtmlSize = 7256;
 
 const litCoreSrc = fs.readFileSync('packages/lit/lit-core.min.js', 'utf8');