fix(observable): backing property should not be enumerable

Fixes: #505.

src/decorator-observable.js

@@ -9,6 +9,11 @@ export function observable(targetOrConfig: any, key: string, descriptor?: Proper
 
     // use a convention to compute the inner property name
     let innerPropertyName = `_${key}`;
+    const innerPropertyDescriptor: PropertyDescriptor = {
+      configurable: true,
+      enumerable: false,
+      writable: true,
+    };
 
     // determine callback name based on config or convention.
     const callbackName = (config && config.changeHandler) || `${key}Changed`;
@@ -18,7 +23,7 @@ export function observable(targetOrConfig: any, key: string, descriptor?: Proper
 
       // set the initial value of the property if it is defined.
       if (typeof descriptor.initializer === 'function') {
-        target[innerPropertyName] = descriptor.initializer();
+        innerPropertyDescriptor.value = descriptor.initializer();
       }
     } else {
       // there is no descriptor if the target was a field in TS (although Babel provides one),
@@ -35,11 +40,18 @@ export function observable(targetOrConfig: any, key: string, descriptor?: Proper
     delete descriptor.writable;
     delete descriptor.initializer;
 
+    // Add the inner property on the prototype.
+    Reflect.defineProperty(target, innerPropertyName, innerPropertyDescriptor);
+
     // add the getter and setter to the property descriptor.
     descriptor.get = function() { return this[innerPropertyName]; };
     descriptor.set = function(newValue) {
       let oldValue = this[innerPropertyName];
+
+      // Add the inner property on the instance and make it nonenumerable.
       this[innerPropertyName] = newValue;
+      Reflect.defineProperty(this, innerPropertyName, { enumerable: false });
+
       if (this[callbackName]) {
         this[callbackName](newValue, oldValue, key);
       }

test/decorator-observable.spec.js

@@ -1,6 +1,8 @@
 import './setup';
 import {observable} from '../src/decorator-observable.js';
 import {decorators} from 'aurelia-metadata';
+import {SetterObserver} from '../src/property-observation';
+import {Logger} from 'aurelia-logging';
 
 describe('observable decorator', () => {
   const oldValue = 'old';
@@ -113,10 +115,94 @@ describe('observable decorator', () => {
     const instance = new class {
       @observable value;
     };
-    const keys = [];
-    for (let p in instance) {
-      keys.push(p);
-    }
-    expect(keys).toContain('value');
+    const descriptor = Object.getOwnPropertyDescriptor(Object.getPrototypeOf(instance), 'value');
+
+    expect(descriptor.enumerable).toBe(true);
+    expect(typeof descriptor.set).toBe('function');
+    expect(typeof descriptor.get).toBe('function');
+  });
+
+  describe('private property', () => {
+    describe(`when public property's value is not changed`, () => {
+      const instance = new class {
+        @observable value;
+      };
+      const prototype = Object.getPrototypeOf(instance);
+
+      it('should exist on the prototype', () => {
+        expect(prototype.hasOwnProperty('_value')).toBe(true);
+      });
+
+      it('should be nonenumerable', () => {
+        expect(prototype.propertyIsEnumerable('_value')).toBe(false);
+      });
+
+      describe('observation', () => {
+        const observer = new SetterObserver(null, prototype, '_value');
+
+        it('should convert to accessors without warning', () => {
+          spyOn(Logger.prototype, 'warn');
+          observer.convertProperty();
+          expect(Logger.prototype.warn).not.toHaveBeenCalled();
+        });
+
+        it('should exist', () => {
+          const descriptor = Object.getOwnPropertyDescriptor(prototype, '_value');
+          expect(typeof descriptor.get).toBe('function');
+          expect(typeof descriptor.set).toBe('function');
+        });
+
+        it('should be nonenumerable', () => {
+          expect(prototype.propertyIsEnumerable('_value')).toBe(false);
+        });
+      });
+    });
+
+    describe(`when public property's value is changed`, () => {
+      const instance = new class {
+        @observable value;
+      };
+      instance.value = newValue;
+
+      it('should exist on the instance', () => {
+        expect(instance.hasOwnProperty('_value')).toBe(true);
+      });
+
+      it('should be nonenumerable', () => {
+        expect(instance.propertyIsEnumerable('_value')).toBe(false);
+      });
+
+      describe('observation', () => {
+        const observer = new SetterObserver(null, instance, '_value');
+
+        it('should convert to accessors without warning', () => {
+          spyOn(Logger.prototype, 'warn');
+          observer.convertProperty();
+          expect(Logger.prototype.warn).not.toHaveBeenCalled();
+        });
+
+        it('should exist', () => {
+          const descriptor = Object.getOwnPropertyDescriptor(instance, '_value');
+          expect(typeof descriptor.get).toBe('function');
+          expect(typeof descriptor.set).toBe('function');
+        });
+
+        it('should be nonenumerable', () => {
+          expect(instance.propertyIsEnumerable('_value')).toBe(false);
+        });
+      });
+    });
+
+    it('should have distinct values between instances', () => {
+      class MyClass {
+        @observable value = oldValue;
+      }
+
+      const instance1 = new MyClass();
+      const instance2 = new MyClass();
+
+      instance1.value = newValue;
+      expect(instance2.value).toBe(oldValue);
+    });
   });
 });