defineReactive doesn't handle inherited properties

[Bernardo-Castilho]

The defineReactive function uses getOwnPropertyDescriptor, which works only for properties declared on the object itself, but not on its ancestors:

/**

• Define a reactive property on an Object.
  */
  function defineReactive (obj, key, val, customSetter) {
  var dep = new Dep()

  var property = Object.getOwnPropertyDescriptor(obj, key)
  if (property && property.configurable === false) {
  return
  }

IMHO, this should be:

/**

• Define a reactive property on an Object.
  */
  function defineReactive(obj, key, val, customSetter) {

  // get property descriptor (own or inherited!)
  var property = null;
  for (var proto = obj; !property && proto != Object.prototype; proto = Object.getPrototypeOf(proto)){
  property = Object.getOwnPropertyDescriptor(proto, key);
  }
  if (property && property.configurable === false) {
  return;
  }

[LinusBorg]

Vue intentionally only extends the properties of plain objects.

[Bernardo-Castilho]

Sorry, I don't follow. The defineReactive code has a comment that says

// cater for pre-defined getter/setters

Which presumably means it will call/honor pre-defined getters and setters. Which it does, but only for properties defined by the object itself, and not for derived classes.

I made two fiddles that compare the Vue1 and 2 binding behavior:

• http://jsfiddle.net/Wijmo5/q3nc6qrw/ (Vue 1, property getters/setters invoked correctly)
• http://jsfiddle.net/Wijmo5/b2022cmy/ (vue 2, property getters/setters not invoked at all)

This is a huge breaking change. It causes the internal fields that back property getters/setters to get out of sync with the model, and the result is a mess for anything but the simplest models.

Perhaps I am doing something wrong here, but this is seems like a huge breaking change. If I can't get the Vue 1 behavior back, I will not be able to migrate my apps to Vue 2.

And as I pointed out earlier (and in the fiddles), the fix seems really simple.

[LinusBorg]

This may have sort of worked in Vue 1, but it was never meant to. The 1.0 API docs for data clearly state (emphasis mine):

The data object for the Vue instance. Vue.js will recursively convert its properties into getter/setters to make it “reactive”. The object must be plain: native objects, existing getter/setters and prototype properties are ignored. It is not recommended to observe complex objects.

[LinusBorg]

@yyx990803 can you chime in about the design decision made here?

[posva]

This may be interesting for Typescript. cc @ktsn

[Bernardo-Castilho]

This is a shame. If that is indeed the case, we might not be able to support Vue 2 at all. It will be useful only for trivial binding scenarios.

It is strange that the "defineReactive" specifically gets the property description and "caters" to the original getter/setter by invoking them exactly as it should:

Object.defineProperty(obj, key, {
enumerable: true,
configurable: true,
get: function reactiveGetter () {
var value = getter ? getter.call(obj) : val // CALLING ORIGINAL GETTER
///...
return value
},
set: function reactiveSetter (newVal) {
/// ...
if (setter) {
setter.call(obj, newVal) // CALLING ORIGINAL SETTER
} else {
val = newVal
}

I don't know much about the intent or original design, but the code seems clear enough, and the line that gets the property by calling getOwnPropertyDefinition without looking at derived classes seems like a bug. I mean, is there any reason at all to get property descriptors defined by the class and not by its ancestors?

Our specific goal is to wrap UI controls into Vue components. This is easy to do in Vue 1, Angular 1, Angular 2, React, and even KnockoutJS. But if our Vue 2 components can't have data members with getters/setters, then I don't see how we could do this. Perhaps with computed properties? But that would seem like a lot of extra work and potential for errors and confusion.

Like I said, it's hard to understand. Seems like a silly bug that can be easily fixed and tested. This would open a whole lot of practical scenarios, and improve compatibility with Vue 1 applications.

[Bernardo-Castilho]

Note: Although we are using TypeScript, the core of this discussion is the issue with getters/setters and inheritance. This applies to ES6, which is just around the corner. IMHO, not fully supporting getters/setters/classes/inheritance would be a huge handicap for the framework.

[Bernardo-Castilho]

For the record, we do have Vue 1.x wrappers for our controls, and they work great. We also have at least five sample apps that use the controls extensively in real-world scenarios.

Today I ported all five samples to Vue 2.x. Mainly, I had to remove the .sync bindings and replace those with change events that update the model. Pretty trivial to do.

All the samples work great as long as I apply the fix mentioned above to the vue.js file. They all break in strange ways if I don't.

I'd be happy to provide copies of the samples to anyone interested. The Vue 1 version will be published on our site next week. Not sure about Vue 2 though... Fingers crossed ;-)

[LinusBorg]

Evan is at a conf right now I'm sure he will chime in when he finds a bit of time.

[Bernardo-Castilho]

Thanks Thorsten. I really want to make this work, love the framework. And it makes me itch to see it so close to working the way we need it to. Just two little lines ;-)

Looking forward to Evan's comments and insights. Perhaps I should change some detail in our implementation.

I am sure as soon as we release our Vue 1 support next week, there will be tons of people asking for our story on Vue 2. I hope to have good news for them!

[yyx990803]

Wow, this is indeed an unintentional breaking change. The real cause is I changed the property enumeration during conversion from Object.keys to for...in during a mass refactor, but that caused the conversion to include all prototype properties as well.

[yyx990803]

It's been addressed in 4c7a87e

Note your demo would still work a bit differently due to the behavior difference of v-model between v1 and v2. However that's a different topic.