Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When Vue has some other global mixins, 'mixins' helper could be dangerous #291

Closed
hikerpig opened this issue Nov 30, 2018 · 4 comments
Closed

Comments

@hikerpig
Copy link

hikerpig commented Nov 30, 2018

I have added a minimal demo in test/test.ts

it('mixins should be safe with VueContructor and global mixins', function () {
    let counter = 0
    /**
     * dangerous global mixin
     */
    Vue.mixin({
        created() {
          counter++
        }
    })
    @Component
    class MixinA extends Vue {
    }
    @Component
    class MixinB extends Vue {
    }
    @Component
    class MyComp extends mixins(MixinA, MixinB) {
      test() {}
    }
    const vm = new MyComp()
    vm.test() // just to avoid 'noUnusedLocals' error
    expect(counter).to.equal(1)  // error, actual 3
})

Global mixin is very common in Vue plugins and real-world projects. With current implementation of 'mixins' helper( or of Vue.extend({ mixins: VueConstructor<any>[] })), there might be some subtle unexpected results.

For example, I have found a vue-i18n caused memory leak in work project

src/mixin.js

Vue.mixin({
    beforeCreate (): void {
      // .... in short ...
      const options: any = this.$options
      options.i18n = options.i18n || (options.__i18n ? {} : null)
      this._i18n = options.i18n || (options.__i18n ? {} : null)

      this._i18n.subscribeDataChanging(this)
    },

    beforeDestroy (): void {
      if (!this._i18n) { return }

      if (this._subscribing) {
        // if two 'beforeCreate' hooks has been added to options, there will be two vm pushed to listener array by '_i18n.subscribeDataChanging'
        // but here, only one '_i18n._i18n.unsubscribeDataChanging' will be called
        // so there is a leak
        this._i18n.unsubscribeDataChanging(this)
      }
      // ...
      this._i18n = null
    }
})

src/index.js

class VueI18n {
  // ...
  subscribeDataChanging (vm: any): void {
    this._dataListeners.push(vm)
  }

  unsubscribeDataChanging (vm: any): void {
    // will only remove the first 'vm' in 'this._dataListeners'
    remove(this._dataListeners, vm)
  }
}
@ktsn
Copy link
Member

ktsn commented Nov 30, 2018

Isn't it the problem that Vue.js does not correctly dedupe the global mixin? It looks happen when we use mixins option directly too. Would you mind to report it to core repo instead?

@ktsn
Copy link
Member

ktsn commented Dec 1, 2018

This was fixed via vuejs/vue#8870

@avxkim
Copy link

avxkim commented May 15, 2019

It's not fixed by the way, versions:

"vue-class-component": "^7.1.0",
"vue-property-decorator": "^8.1.1",

When using created() hook, with global mixins, console.log('123') is fired for every component you have in a project, but if you use mounted() it works as intended.

@hikerpig
Copy link
Author

@heihachi88 It's a core repo bug and is fixed in v2.6.0. There are also some followups in some next patch versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants