fix(observer): invoke getters on initial observation if setter defined. Corrects #7302

[pkaminski]

What kind of change does this PR introduce? (check at least one)

• Bugfix
• Feature
• Code style update
• Refactor
• Build-related changes
• Other, please describe:

Does this PR introduce a breaking change? (check one)

• Yes
• No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

• It's submitted to the dev branch for v2.x (or to a previous version branch), not the master branch
• When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
• All tests are passing: https://github.com/vuejs/vue/blob/dev/.github/CONTRIBUTING.md#development-setup
• New/updated tests are included

If adding a new feature, the PR's description includes:

• A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

After #7302, there's an asymmetry in defineReactive for properties that have both a getter and a setter. On initial creation, the value returned by the getter is not made reactive since #7302 makes sure not to fetch it and observe(val) is a no-op. However, if the value is set again after the property has been made reactive, it is made reactive as well since observe(newVal) in set is not gated on missing a getter. (The premise justifying the change in #7280 that "why should we get the value of the property in walk, despite the fact, that it is used only if the property has no getter" is wrong, as the value was used in the call to observe even if the property had a getter.)

With the present change, properties that only have a getter will still not have their value fetched and won't be made reactive, which should continue to satisfy @DeyLak's needs IIUC. However, properties that have both a getter and a setter will continue to behave like they did before, being made recursively reactive, and their initial value treated consistently with subsequently set ones.

If you think that treating getter vs getter/setter properties differently is confusing, then you may want to revert #7302 instead so all values are made recursively reactive, irrespective of how the property is defined. This may be easier for developers to understand but would break @DeyLak's use case.

[jan-swiecki]

@pkaminski good change. @yyx990803 I think VueJS needs to think through and define properly behaviour for objects that have Object.defineProperty properties. I think this JS feature will become more popular and more people will have various issues (this behaviour is not even documented in VueJS docs). Issues ranging from one side to another (e.g. @pkaminski's use case or @DeyLak's use case).

Example use case that was not mentioned here (I think) is that you can define properties with defineProperty for validation purposes only. In this case you can make object always to be in a proper validated state (setter prohibits wrong state). We can see that this kind of object can be thought as standard object with intention to be used in the UI with all reactive/observable logic. (Disclaimer: this is my use case).

I'm not saying that above is an argument that we should define reactive logic for above use case. I'm just adding this example as an addition to the discussion and I'm saying that this problem should be analysed more, so proper logic can be defined - not necessarily making above use case work.

Evan I know that you are super busy, so when I work more with my use case I may come back here and add some more input from the analysis point of view.

[jan-swiecki]

I will just dump some ideas here when they come.

First finding

export function defineReactive (
  obj: Object,
  key: string,
  val: any,
  customSetter?: ?Function,
  shallow?: boolean
) {
  const dep = new Dep()

  (...)
  Object.defineProperty(obj, key, {
    (...)
    set: function reactiveSetter (newVal) {
      const value = getter ? getter.call(obj) : val
      /* eslint-disable no-self-compare */
      if (newVal === value || (newVal !== newVal && value !== value)) {
        return
      }
      (...)
      if (setter) {
        setter.call(obj, newVal)
      } else {
        val = newVal
      }

Line if (newVal === value || (newVal !== newVal && value !== value)) { breaks behaviour of object that has a setter defined. If we define Object.defineProperty(obj, 'x', ... set: ...) and run obj.x = obj.x then normally setter will run, but defineReactive breaks this logic and prevents setter from running.

One would argue why would anybody run obj.x = obj.x and depend on the logic being run in the setter?

Let's again use validator example (very simplified for readability):

function validateEmail(email) {
  return email !== null && email.match(/^.+@.+/);
}

class User {
  constructor() {
    let email = null;

    Object.defineProperty(this, 'email', {
      get: function() {
        return email;
      },
      set: function(newValue) {
        if(this.mustBeValid) {
          validateEmail(newValue); // throw Error if wrong email format
        }
        email = newValue;
      }
    });

    let mustBeValid = null;

    Object.defineProperty(this, 'mustBeValid', {
      get: function() {
        return mustBeValid;
      },
      set: function(newValue) {
        mustBeValid = newValue;

        // validation is re-enabled
        if(mustBeValid) {
          // re-save to run field validation
          user.email = user.email; // normally we iterate over fields excluding mustBeValid field
        }
      }
    });
  }
}

Vue.component('user-form', {
  template: `
  <form>
    Email
    <input type="text" v-model="user.email">
    <button @click="register()"
  </form>
`,
  data() {
    let user = new User();
    user.mustBeValid = false;
    user.email = '';

    return {
      user: user
    };
  },
  methods: {
    register() {
      user.mustBeValid = true;
      this.$axios.post(...);
    }
  },
});

Above logic can be easily extended into tracking validation errors, custom number of fields with custom validators, displaying of errors automatically and disabling submit button on validation errors etc.

Besides this example I think that object's whole behaviour, even in stupid use cases, should not be modified or broken.