Removes wrapping of properties in favor of always respecting noAccessor

CHANGELOG.md

@@ -18,6 +18,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 ### Fixed
 * Fix jsfiddle reference.
 
+### Changed
+* [Breaking] Property accessors are no longer wrapped when they already exist. Instead the `noAccessor` flag should be set when a user-defined accessor exists on the prototype (and in this case, user-defined accessors must call `requestUpdate` themselves). ([#450](https://github.com/Polymer/lit-element/issues/450)).
+
 ## [2.0.0-rc.2] - 2019-01-11
 ### Fixed
 * Fix references to `@polymer/lit-element` in README and docs ([#427](https://github.com/Polymer/lit-element/pull/427)).

src/lib/decorators.ts

@@ -91,32 +91,45 @@ export const customElement = (tagName: string) => (
 
 const standardProperty =
     (options: PropertyDeclaration, element: ClassElement) => {
-      // createProperty() takes care of defining the property, but we still must
-      // return some kind of descriptor, so return a descriptor for an unused
-      // prototype field. The finisher calls createProperty().
-      return {
-        kind : 'field',
-        key : Symbol(),
-        placement : 'own',
-        descriptor : {},
-        // When @babel/plugin-proposal-decorators implements initializers,
-        // do this instead of the initializer below. See:
-        // https://github.com/babel/babel/issues/9260 extras: [
-        //   {
-        //     kind: 'initializer',
-        //     placement: 'own',
-        //     initializer: descriptor.initializer,
-        //   }
-        // ],
-        initializer(this: any) {
-          if (typeof element.initializer === 'function') {
-            this[element.key] = element.initializer!.call(this);
+      // When decorating an accessor, pass it through and add property metadata.
+      // Note, the `hasOwnProperty` check in `createProperty` ensures we don't
+      // stomp over the user's accessor.
+      if (element.kind === 'method' && element.descriptor &&
+          !('value' in element.descriptor)) {
+        return {
+          ...element,
+          finisher(clazz: typeof UpdatingElement) {
+            clazz.createProperty(element.key, options);
           }
-        },
-        finisher(clazz: typeof UpdatingElement) {
-          clazz.createProperty(element.key, options);
-        }
-      };
+        };
+      } else {
+        // createProperty() takes care of defining the property, but we still
+        // must return some kind of descriptor, so return a descriptor for an
+        // unused prototype field. The finisher calls createProperty().
+        return {
+          kind : 'field',
+          key : Symbol(),
+          placement : 'own',
+          descriptor : {},
+          // When @babel/plugin-proposal-decorators implements initializers,
+          // do this instead of the initializer below. See:
+          // https://github.com/babel/babel/issues/9260 extras: [
+          //   {
+          //     kind: 'initializer',
+          //     placement: 'own',
+          //     initializer: descriptor.initializer,
+          //   }
+          // ],
+          initializer(this: any) {
+            if (typeof element.initializer === 'function') {
+              this[element.key] = element.initializer!.call(this);
+            }
+          },
+          finisher(clazz: typeof UpdatingElement) {
+            clazz.createProperty(element.key, options);
+          }
+        };
+      }
     };
 
 const legacyProperty = (options: PropertyDeclaration, proto: Object,

src/lib/updating-element.ts

@@ -20,24 +20,6 @@
  */
 const JSCompiler_renameProperty = (prop: PropertyKey, _obj: any) => prop;
 
-/**
- * Returns the property descriptor for a property on this prototype by walking
- * up the prototype chain. Note that we stop just before Object.prototype, which
- * also avoids issues with Symbol polyfills (core-js, get-own-property-symbols),
- * which create accessors for the symbols on Object.prototype.
- */
-const descriptorFromPrototype = (name: PropertyKey, proto: UpdatingElement) => {
-  if (name in proto) {
-    while (proto !== Object.prototype) {
-      if (proto.hasOwnProperty(name)) {
-        return Object.getOwnPropertyDescriptor(proto, name);
-      }
-      proto = Object.getPrototypeOf(proto);
-    }
-  }
-  return undefined;
-};
-
 /**
  * Converts property values to and from attribute values.
  */
@@ -288,37 +270,26 @@ export abstract class UpdatingElement extends HTMLElement {
     // metadata.
     this._ensureClassProperties();
     this._classProperties!.set(name, options);
-    if (!options.noAccessor) {
-      const superDesc = descriptorFromPrototype(name, this.prototype);
-      let desc;
-      // If there is a super accessor, capture it and "super" to it
-      if (superDesc !== undefined && (superDesc.set && superDesc.get)) {
-        const {set, get} = superDesc;
-        desc = {
-          get() { return get.call(this); },
-          set(value: any) {
-            const oldValue = this[name];
-            set.call(this, value);
-            this.requestUpdate(name, oldValue);
-          },
-          configurable : true,
-          enumerable : true
-        };
-      } else {
-        const key = typeof name === 'symbol' ? Symbol() : `__${name}`;
-        desc = {
-          get() { return this[key]; },
-          set(value: any) {
-            const oldValue = this[name];
-            this[key] = value;
-            this.requestUpdate(name, oldValue);
-          },
-          configurable : true,
-          enumerable : true
-        };
-      }
-      Object.defineProperty(this.prototype, name, desc);
+    // 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)) {
+      return;
     }
+    const key = typeof name === 'symbol' ? Symbol() : `__${name}`;
+    Object.defineProperty(this.prototype, name, {
+      get(): any { return (this as any)[key]; },
+      set(this: UpdatingElement, value: any) {
+        const oldValue = (this as any)[name];
+        (this as any)[key] = value;
+          this.requestUpdate(name, oldValue);
+        },
+        configurable : true,
+        enumerable : true
+      }
+    );
   }
 
   /**

src/test/lib/decorators_test.ts

@@ -185,6 +185,39 @@ suite('decorators', () => {
       assert.equal(el.updateCount, 6);
     });
 
+    test('can decorate user accessor with @property', async () => {
+      class E extends LitElement {
+
+        _foo?: number;
+        updatedContent?: number;
+
+        @property({reflect : true, type: Number})
+        get foo() {
+          return this._foo as number;
+        }
+
+        set foo(v: number) {
+          const old = this.foo;
+          this._foo = v;
+          this.requestUpdate('foo', old);
+        }
+
+        updated() { this.updatedContent = this.foo; }
+      }
+      customElements.define(generateElementName(), E);
+      const el = new E();
+      container.appendChild(el);
+      await el.updateComplete;
+      assert.equal(el._foo, undefined);
+      assert.equal(el.updatedContent, undefined);
+      assert.isFalse(el.hasAttribute('foo'));
+      el.foo = 5;
+      await el.updateComplete;
+      assert.equal(el._foo, 5);
+      assert.equal(el.updatedContent, 5);
+      assert.equal(el.getAttribute('foo'), '5');
+    });
+
     test('can mix property options via decorator and via getter', async () => {
       const hasChanged = (value: any, old: any) =>
           old === undefined || value > old;