Added handling and tests for nonconfigurable properties, get-only properties, and set-only properties

Author: jdavidberger

src/observer/index.js

@@ -173,26 +173,36 @@ function copyAugment (target, src, keys) {
 
 function defineReactive (obj, key, val) {
   var dep = new Dep()
+  var hasGetter = true
+  var hasSetter = true
 
   var target = {
     val: val
   }
 
   if (config.convertAllProperties) {
     var property = Object.getOwnPropertyDescriptor(obj, key)
-    if (property && property.get && property.set) {
+    if (property && property.configurable === false) {
+      return
+    }
+    if (property && (property.get || property.set)) {
+      hasGetter = property.get !== undefined
+      hasSetter = property.set !== undefined
       Object.defineProperty(target, 'val', {
-        get: _.bind(property.get, obj),
-        set: _.bind(property.set, obj)
+        get: property.get && _.bind(property.get, obj),
+        set: property.set && _.bind(property.set, obj)
       })
     }
   }
 
   var childOb = Observer.create(target.val)
-  Object.defineProperty(obj, key, {
+  var propertyDefinition = {
     enumerable: true,
-    configurable: true,
-    get: function metaGetter () {
+    configurable: true
+  }
+
+  if (hasGetter) {
+    propertyDefinition.get = function metaGetter () {
       var val = target.val
       if (Dep.target) {
         dep.depend()
@@ -206,15 +216,20 @@ function defineReactive (obj, key, val) {
           }
         }
       }
-      return target.val
-    },
-    set: function metaSetter (newVal) {
+      return val
+    }
+  }
+
+  if (hasSetter) {
+    propertyDefinition.set = function metaSetter (newVal) {
       if (newVal === target.val) return
       target.val = newVal
       childOb = Observer.create(newVal)
       dep.notify()
     }
-  })
+  }
+
+  Object.defineProperty(obj, key, propertyDefinition)
 }
 
 // Attach to the util object so it can be used elsewhere.

test/unit/specs/observer/observer_spec.js

@@ -46,10 +46,12 @@ describe('Observer', function () {
     // on object
     var obj = {}
     var val = 0
+    var getCount = 0
     Object.defineProperty(obj, 'a', {
       configurable: true,
       enumerable: true,
       get: function () {
+        getCount++
         return val
       },
       set: function (v) {
@@ -62,6 +64,13 @@ describe('Observer', function () {
     expect(ob.value).toBe(obj)
     expect(obj.__ob__).toBe(ob)
 
+    getCount = 0
+    // Each read of 'a' should result in only one get underlying get call
+    obj.a
+    expect(getCount).toBe(1)
+    obj.a
+    expect(getCount).toBe(2)
+
     // should return existing ob on already observed objects
     var ob2 = Observer.create(obj)
     expect(ob2).toBe(ob)
@@ -73,6 +82,95 @@ describe('Observer', function () {
     config.convertAllProperties = previousConvertAllProperties
   })
 
+  it('create on property with only getter', function () {
+    var previousConvertAllProperties = config.convertAllProperties
+    config.convertAllProperties = true
+
+    // on object
+    var obj = {}
+    Object.defineProperty(obj, 'a', {
+      configurable: true,
+      enumerable: true,
+      get: function () {
+        return 123
+      }
+    })
+
+    var ob = Observer.create(obj)
+    expect(ob instanceof Observer).toBe(true)
+    expect(ob.value).toBe(obj)
+    expect(obj.__ob__).toBe(ob)
+
+    // should be able to read
+    expect(obj.a).toBe(123)
+
+    // should return existing ob on already observed objects
+    var ob2 = Observer.create(obj)
+    expect(ob2).toBe(ob)
+
+    // since there is no setter, you shouldn't be able to write to it
+    expect(function () {
+      obj.a = 101
+    }).toThrow()
+    expect(obj.a).toBe(123)
+
+    config.convertAllProperties = previousConvertAllProperties
+  })
+
+  it('create on property with only setter', function () {
+    var previousConvertAllProperties = config.convertAllProperties
+    config.convertAllProperties = true
+
+    // on object
+    var obj = {}
+    var val = 10
+    Object.defineProperty(obj, 'a', { // eslint-disable-line accessor-pairs
+      configurable: true,
+      enumerable: true,
+      set: function (v) {
+        val = v
+      }
+    })
+
+    var ob = Observer.create(obj)
+    expect(ob instanceof Observer).toBe(true)
+    expect(ob.value).toBe(obj)
+    expect(obj.__ob__).toBe(ob)
+
+    // reads should return undefined
+    expect(obj.a).toBe(undefined)
+
+    // should return existing ob on already observed objects
+    var ob2 = Observer.create(obj)
+    expect(ob2).toBe(ob)
+
+    // writes should call the set function
+    obj.a = 100
+    expect(val).toBe(100)
+
+    config.convertAllProperties = previousConvertAllProperties
+  })
+
+  it('create on property which is marked not configurable', function () {
+    var previousConvertAllProperties = config.convertAllProperties
+    config.convertAllProperties = true
+
+    // on object
+    var obj = {}
+    Object.defineProperty(obj, 'a', {
+      configurable: false,
+      enumerable: true,
+      val: 10
+    })
+
+    var ob = Observer.create(obj)
+    expect(ob instanceof Observer).toBe(true)
+    expect(ob.value).toBe(obj)
+    expect(obj.__ob__).toBe(ob)
+
+    config.convertAllProperties = previousConvertAllProperties
+  })
+
   it('create on array', function () {
     // on object
     var arr = [{}, {}]