fix(runtime-core): mixin options that rely on this context should be deferred

Also ensure consistent option apply order with Vue 2, close #1016, close #1029

packages/runtime-core/__tests__/apiOptions.spec.ts

@@ -562,6 +562,51 @@ describe('api: options', () => {
     expect(serializeInner(root)).toBe(`<div>1,1,3</div>`)
   })
 
+  // #1016
+  test('watcher initialization should be deferred in mixins', async () => {
+    const mixin1 = {
+      data() {
+        return {
+          mixin1Data: 'mixin1'
+        }
+      },
+      methods: {}
+    }
+
+    const watchSpy = jest.fn()
+    const mixin2 = {
+      watch: {
+        mixin3Data: watchSpy
+      }
+    }
+
+    const mixin3 = {
+      data() {
+        return {
+          mixin3Data: 'mixin3'
+        }
+      },
+      methods: {}
+    }
+
+    let vm: any
+    const Comp = {
+      mixins: [mixin1, mixin2, mixin3],
+      render() {},
+      created() {
+        vm = this
+      }
+    }
+
+    const root = nodeOps.createElement('div')
+    render(h(Comp), root)
+
+    // should have no warnings
+    vm.mixin3Data = 'hello'
+    await nextTick()
+    expect(watchSpy.mock.calls[0].slice(0, 2)).toEqual(['hello', 'mixin3'])
+  })
+
   describe('warnings', () => {
     mockWarn()
 
@@ -631,53 +676,34 @@ describe('api: options', () => {
       ).toHaveBeenWarned()
     })
 
-    test('data property is already declared in props', () => {
-      const Comp = {
-        props: { foo: Number },
-        data: () => ({
-          foo: 1
-        }),
-        render() {}
-      }
-
-      const root = nodeOps.createElement('div')
-      render(h(Comp), root)
-      expect(
-        `Data property "foo" is already defined in Props.`
-      ).toHaveBeenWarned()
-    })
-
-    test('computed property is already declared in data', () => {
+    test('inject property is already declared in props', () => {
       const Comp = {
-        data: () => ({
-          foo: 1
-        }),
-        computed: {
-          foo() {}
+        data() {
+          return {
+            a: 1
+          }
         },
-        render() {}
-      }
-
-      const root = nodeOps.createElement('div')
-      render(h(Comp), root)
-      expect(
-        `Computed property "foo" is already defined in Data.`
-      ).toHaveBeenWarned()
-    })
-
-    test('computed property is already declared in props', () => {
-      const Comp = {
-        props: { foo: Number },
-        computed: {
-          foo() {}
+        provide() {
+          return {
+            a: this.a
+          }
         },
-        render() {}
-      }
+        render() {
+          return [h(ChildA)]
+        }
+      } as any
+      const ChildA = {
+        props: { a: Number },
+        inject: ['a'],
+        render() {
+          return this.a
+        }
+      } as any
 
       const root = nodeOps.createElement('div')
       render(h(Comp), root)
       expect(
-        `Computed property "foo" is already defined in Props.`
+        `Inject property "a" is already defined in Props.`
       ).toHaveBeenWarned()
     })
 
@@ -697,11 +723,11 @@ describe('api: options', () => {
       ).toHaveBeenWarned()
     })
 
-    test('methods property is already declared in data', () => {
+    test('methods property is already declared in props', () => {
       const Comp = {
-        data: () => ({
-          foo: 2
-        }),
+        props: {
+          foo: Number
+        },
         methods: {
           foo() {}
         },
@@ -711,50 +737,60 @@ describe('api: options', () => {
       const root = nodeOps.createElement('div')
       render(h(Comp), root)
       expect(
-        `Methods property "foo" is already defined in Data.`
+        `Methods property "foo" is already defined in Props.`
       ).toHaveBeenWarned()
     })
 
-    test('methods property is already declared in props', () => {
+    test('methods property is already declared in inject', () => {
       const Comp = {
-        props: {
-          foo: Number
+        data() {
+          return {
+            a: 1
+          }
+        },
+        provide() {
+          return {
+            a: this.a
+          }
         },
+        render() {
+          return [h(ChildA)]
+        }
+      } as any
+      const ChildA = {
         methods: {
-          foo() {}
+          a: () => null
         },
-        render() {}
-      }
+        inject: ['a'],
+        render() {
+          return this.a
+        }
+      } as any
 
       const root = nodeOps.createElement('div')
       render(h(Comp), root)
       expect(
-        `Methods property "foo" is already defined in Props.`
+        `Methods property "a" is already defined in Inject.`
       ).toHaveBeenWarned()
     })
 
-    test('methods property is already declared in computed', () => {
+    test('data property is already declared in props', () => {
       const Comp = {
-        computed: {
-          foo: {
-            get() {},
-            set() {}
-          }
-        },
-        methods: {
-          foo() {}
-        },
+        props: { foo: Number },
+        data: () => ({
+          foo: 1
+        }),
         render() {}
       }
 
       const root = nodeOps.createElement('div')
       render(h(Comp), root)
       expect(
-        `Methods property "foo" is already defined in Computed.`
+        `Data property "foo" is already defined in Props.`
       ).toHaveBeenWarned()
     })
 
-    test('inject property is already declared in data', () => {
+    test('data property is already declared in inject', () => {
       const Comp = {
         data() {
           return {
@@ -785,42 +821,45 @@ describe('api: options', () => {
       const root = nodeOps.createElement('div')
       render(h(Comp), root)
       expect(
-        `Inject property "a" is already defined in Data.`
+        `Data property "a" is already defined in Inject.`
       ).toHaveBeenWarned()
     })
 
-    test('inject property is already declared in props', () => {
+    test('data property is already declared in methods', () => {
       const Comp = {
-        data() {
-          return {
-            a: 1
-          }
+        data: () => ({
+          foo: 1
+        }),
+        methods: {
+          foo() {}
         },
-        provide() {
-          return {
-            a: this.a
-          }
+        render() {}
+      }
+
+      const root = nodeOps.createElement('div')
+      render(h(Comp), root)
+      expect(
+        `Data property "foo" is already defined in Methods.`
+      ).toHaveBeenWarned()
+    })
+
+    test('computed property is already declared in props', () => {
+      const Comp = {
+        props: { foo: Number },
+        computed: {
+          foo() {}
         },
-        render() {
-          return [h(ChildA)]
-        }
-      } as any
-      const ChildA = {
-        props: { a: Number },
-        inject: ['a'],
-        render() {
-          return this.a
-        }
-      } as any
+        render() {}
+      }
 
       const root = nodeOps.createElement('div')
       render(h(Comp), root)
       expect(
-        `Inject property "a" is already defined in Props.`
+        `Computed property "foo" is already defined in Props.`
       ).toHaveBeenWarned()
     })
 
-    test('inject property is already declared in computed', () => {
+    test('computed property is already declared in inject', () => {
       const Comp = {
         data() {
           return {
@@ -852,40 +891,43 @@ describe('api: options', () => {
       const root = nodeOps.createElement('div')
       render(h(Comp), root)
       expect(
-        `Inject property "a" is already defined in Computed.`
+        `Computed property "a" is already defined in Inject.`
       ).toHaveBeenWarned()
     })
 
-    test('inject property is already declared in methods', () => {
+    test('computed property is already declared in methods', () => {
       const Comp = {
-        data() {
-          return {
-            a: 1
-          }
-        },
-        provide() {
-          return {
-            a: this.a
-          }
+        computed: {
+          foo() {}
         },
-        render() {
-          return [h(ChildA)]
-        }
-      } as any
-      const ChildA = {
         methods: {
-          a: () => null
+          foo() {}
         },
-        inject: ['a'],
-        render() {
-          return this.a
-        }
-      } as any
+        render() {}
+      }
 
       const root = nodeOps.createElement('div')
       render(h(Comp), root)
       expect(
-        `Inject property "a" is already defined in Methods.`
+        `Computed property "foo" is already defined in Methods.`
+      ).toHaveBeenWarned()
+    })
+
+    test('computed property is already declared in data', () => {
+      const Comp = {
+        data: () => ({
+          foo: 1
+        }),
+        computed: {
+          foo() {}
+        },
+        render() {}
+      }
+
+      const root = nodeOps.createElement('div')
+      render(h(Comp), root)
+      expect(
+        `Computed property "foo" is already defined in Data.`
       ).toHaveBeenWarned()
     })
   })

packages/runtime-core/src/componentOptions.ts

@@ -39,7 +39,8 @@ import {
 import {
   reactive,
   ComputedGetter,
-  WritableComputedOptions
+  WritableComputedOptions,
+  toRaw
 } from '@vue/reactivity'
 import {
   ComponentObjectPropsOptions,
@@ -260,12 +261,15 @@ function createDuplicateChecker() {
   }
 }
 
+type DataFn = (vm: ComponentPublicInstance) => any
+
 export function applyOptions(
   instance: ComponentInternalInstance,
   options: ComponentOptions,
+  deferredData: DataFn[] = [],
+  deferredWatch: ComponentWatchOptions[] = [],
   asMixin: boolean = false
 ) {
-  const publicThis = instance.proxy!
   const {
     // composition
     mixins,
@@ -295,6 +299,7 @@ export function applyOptions(
     errorCaptured
   } = options
 
+  const publicThis = instance.proxy!
   const ctx = instance.ctx
   const globalMixins = instance.appContext.mixins
   // call it only during dev
@@ -303,15 +308,15 @@ export function applyOptions(
   if (!asMixin) {
     callSyncHook('beforeCreate', options, publicThis, globalMixins)
     // global mixins are applied first
-    applyMixins(instance, globalMixins)
+    applyMixins(instance, globalMixins, deferredData, deferredWatch)
   }
   // extending a base component...
   if (extendsOptions) {
-    applyOptions(instance, extendsOptions, true)
+    applyOptions(instance, extendsOptions, deferredData, deferredWatch, true)
   }
   // local mixins
   if (mixins) {
-    applyMixins(instance, mixins)
+    applyMixins(instance, mixins, deferredData, deferredWatch)
   }
 
   const checkDuplicateProperties = __DEV__ ? createDuplicateChecker() : null
@@ -322,7 +327,55 @@ export function applyOptions(
     }
   }
 
-  // state options
+  // options initialization order (to be consistent with Vue 2):
+  // - props (already done outside of this function)
+  // - inject
+  // - methods
+  // - data (deferred since it relies on `this` access)
+  // - computed
+  // - watch (deferred since it relies on `this` access)
+
+  if (injectOptions) {
+    if (isArray(injectOptions)) {
+      for (let i = 0; i < injectOptions.length; i++) {
+        const key = injectOptions[i]
+        ctx[key] = inject(key)
+        if (__DEV__) {
+          checkDuplicateProperties!(OptionTypes.INJECT, key)
+        }
+      }
+    } else {
+      for (const key in injectOptions) {
+        const opt = injectOptions[key]
+        if (isObject(opt)) {
+          ctx[key] = inject(opt.from, opt.default)
+        } else {
+          ctx[key] = inject(opt)
+        }
+        if (__DEV__) {
+          checkDuplicateProperties!(OptionTypes.INJECT, key)
+        }
+      }
+    }
+  }
+
+  if (methods) {
+    for (const key in methods) {
+      const methodHandler = (methods as MethodOptions)[key]
+      if (isFunction(methodHandler)) {
+        ctx[key] = methodHandler.bind(publicThis)
+        if (__DEV__) {
+          checkDuplicateProperties!(OptionTypes.METHODS, key)
+        }
+      } else if (__DEV__) {
+        warn(
+          `Method "${key}" has type "${typeof methodHandler}" in the component definition. ` +
+            `Did you reference the function correctly?`
+        )
+      }
+    }
+  }
+
   if (dataOptions) {
     if (__DEV__ && !isFunction(dataOptions)) {
       warn(
@@ -330,33 +383,29 @@ export function applyOptions(
           `Plain object usage is no longer supported.`
       )
     }
-    const data = dataOptions.call(publicThis, publicThis)
-    if (__DEV__ && isPromise(data)) {
-      warn(
-        `data() returned a Promise - note data() cannot be async; If you ` +
-          `intend to perform data fetching before component renders, use ` +
-          `async setup() + <Suspense>.`
-      )
+
+    if (asMixin) {
+      deferredData.push(dataOptions as DataFn)
+    } else {
+      resolveData(instance, dataOptions, publicThis)
     }
-    if (!isObject(data)) {
-      __DEV__ && warn(`data() should return an object.`)
-    } else if (instance.data === EMPTY_OBJ) {
-      if (__DEV__) {
-        for (const key in data) {
-          checkDuplicateProperties!(OptionTypes.DATA, key)
-          // expose data on ctx during dev
-          Object.defineProperty(ctx, key, {
-            configurable: true,
-            enumerable: true,
-            get: () => data[key],
-            set: NOOP
-          })
-        }
+  }
+  if (!asMixin) {
+    if (deferredData.length) {
+      deferredData.forEach(dataFn => resolveData(instance, dataFn, publicThis))
+    }
+    if (__DEV__) {
+      const rawData = toRaw(instance.data)
+      for (const key in rawData) {
+        checkDuplicateProperties!(OptionTypes.DATA, key)
+        // expose data on ctx during dev
+        Object.defineProperty(ctx, key, {
+          configurable: true,
+          enumerable: true,
+          get: () => rawData[key],
+          set: NOOP
+        })
       }
-      instance.data = reactive(data)
-    } else {
-      // existing data: this is a mixin or extends.
-      extend(instance.data, data)
     }
   }
 
@@ -397,27 +446,15 @@ export function applyOptions(
     }
   }
 
-  if (methods) {
-    for (const key in methods) {
-      const methodHandler = (methods as MethodOptions)[key]
-      if (isFunction(methodHandler)) {
-        ctx[key] = methodHandler.bind(publicThis)
-        if (__DEV__) {
-          checkDuplicateProperties!(OptionTypes.METHODS, key)
-        }
-      } else if (__DEV__) {
-        warn(
-          `Method "${key}" has type "${typeof methodHandler}" in the component definition. ` +
-            `Did you reference the function correctly?`
-        )
-      }
-    }
-  }
-
   if (watchOptions) {
-    for (const key in watchOptions) {
-      createWatcher(watchOptions[key], ctx, publicThis, key)
-    }
+    deferredWatch.push(watchOptions)
+  }
+  if (!asMixin && deferredWatch.length) {
+    deferredWatch.forEach(watchOptions => {
+      for (const key in watchOptions) {
+        createWatcher(watchOptions[key], ctx, publicThis, key)
+      }
+    })
   }
 
   if (provideOptions) {
@@ -429,30 +466,6 @@ export function applyOptions(
     }
   }
 
-  if (injectOptions) {
-    if (isArray(injectOptions)) {
-      for (let i = 0; i < injectOptions.length; i++) {
-        const key = injectOptions[i]
-        ctx[key] = inject(key)
-        if (__DEV__) {
-          checkDuplicateProperties!(OptionTypes.INJECT, key)
-        }
-      }
-    } else {
-      for (const key in injectOptions) {
-        const opt = injectOptions[key]
-        if (isObject(opt)) {
-          ctx[key] = inject(opt.from, opt.default)
-        } else {
-          ctx[key] = inject(opt)
-        }
-        if (__DEV__) {
-          checkDuplicateProperties!(OptionTypes.INJECT, key)
-        }
-      }
-    }
-  }
-
   // asset options
   if (components) {
     extend(instance.components, components)
@@ -536,10 +549,35 @@ function callHookFromMixins(
 
 function applyMixins(
   instance: ComponentInternalInstance,
-  mixins: ComponentOptions[]
+  mixins: ComponentOptions[],
+  deferredData: DataFn[],
+  deferredWatch: ComponentWatchOptions[]
 ) {
   for (let i = 0; i < mixins.length; i++) {
-    applyOptions(instance, mixins[i], true)
+    applyOptions(instance, mixins[i], deferredData, deferredWatch, true)
+  }
+}
+
+function resolveData(
+  instance: ComponentInternalInstance,
+  dataFn: DataFn,
+  publicThis: ComponentPublicInstance
+) {
+  const data = dataFn.call(publicThis, publicThis)
+  if (__DEV__ && isPromise(data)) {
+    warn(
+      `data() returned a Promise - note data() cannot be async; If you ` +
+        `intend to perform data fetching before component renders, use ` +
+        `async setup() + <Suspense>.`
+    )
+  }
+  if (!isObject(data)) {
+    __DEV__ && warn(`data() should return an object.`)
+  } else if (instance.data === EMPTY_OBJ) {
+    instance.data = reactive(data)
+  } else {
+    // existing data: this is a mixin or extends.
+    extend(instance.data, data)
   }
 }