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

Mutations of data in arrow functions declared as instance variables are ignored #67

Closed
kimamula opened this issue Feb 25, 2017 · 9 comments
Labels

Comments

@kimamula
Copy link

In the following example, mutations of this.width and this.height in onResize do not actually change the size of the div element.

<template>
<div v-bind:style="{ background: 'red', width: width + 'px', height: height + 'px' }"></div>
</template>

<script>
import Vue from 'vue'
import Component from 'vue-class-component'

@Component
export default class FollowWindowSize extends Vue {
  width = window.innerWidth
  height = window.innerHeight
  onResize = () => {
    this.width = window.innerWidth
    this.height = window.innerHeight
  }

  mounted() {
    window.addEventListener('resize', this.onResize)
  }

  destroyed() {
    window.removeEventListener('resize', this.onResize)
  }
}
</script>

This is because, after the babel transpilation, onResize is declared inside constructor, which is executed before the wrapping of the data with getters and setters by Vue.js take place, and therefore mutations of data in onResize are not handled by these setters.
I confirmed that this is also the case when TypeScript is used.

@ktsn
Copy link
Member

ktsn commented Feb 26, 2017

Thank you for reporting this issue.
Currently, we are doing some hacks to collect class properties as Vue instance data and it seems the cause of this problem. In fact, this in onResize function does not point to the Vue instance.

I'm not sure if we can solve this problem. But we can work around this problem by just using methods instead of arrow function. Note that Vue binds vm instance as this automatically, so we does not have to manually bind them.

import Vue from 'vue'
import Component from 'vue-class-component'

@Component
export default class FollowWindowSize extends Vue {
  width = window.innerWidth
  height = window.innerHeight

  onResize() {
    this.width = window.innerWidth
    this.height = window.innerHeight
  }

  mounted() {
    window.addEventListener('resize', this.onResize)
  }

  destroyed() {
    window.removeEventListener('resize', this.onResize)
  }
}

@HerringtonDarkholme
Copy link
Member

HerringtonDarkholme commented Feb 26, 2017

I don't think this is fixable. Recommended solution is just declaring onResize as method. Vue will automatically bind this to instance.

The reason we cannot support this is below:

Class syntax does not allow this to be injected to constructor. This is specified by ES6 spec.
On the other hand, vue-class-component (and friends) collect instance properties initialized in constructor, and wrap them in data option.

The onResize is declared as property, so it will be initialized in constructor and will be collected as data option. However, to make it work, this must be the vue instance. This violates ES6 spec.

@ktsn
Copy link
Member

ktsn commented Feb 26, 2017

As @HerringtonDarkholme explained, this is unfixable. I think we should mention about this behavior in docs.

@kimamula
Copy link
Author

vue-class-component (and friends) collect instance properties initialized in constructor, and wrap them in data option.

Isn't it possible to change this behavior?

I think there are a few problems resulting from this behavior.

  • vue-class-component has to create a component instance to collect its data
  • Sometimes you may want to prevent some of the instance variable from being handled as data, but you can't.
  • A component instance is not actually created by new ComponentClass(), which can possibly cause unpredictable bugs in some cases.
    • This may be surprising for users, too.

With the following modification of vue-class-component,

https://github.com/kimamula/vue-class-component/pull/1/files

FollowWindowSize can be implemented as follows.

@Component({ data: () => ({
  width: window.innerWidth,
  height: window.innerHeight
}) })
export default class FollowWindowSize extends Vue {
  onResize = () => {
    this.width = window.innerWidth
    this.height = window.innerHeight
  }

  mounted() {
    window.addEventListener('resize', this.onResize)
  }

  destroyed() {
    window.removeEventListener('resize', this.onResize)
  }
}

This implementation resolves the problems listed above.
What do you think of it?

It is possible to change the behavior depending on whether data is included in the argument, which can provide backward compatibility.

@kimamula
Copy link
Author

I also want to note that #68 is also resolved in the above implementation.

@ktsn
Copy link
Member

ktsn commented Mar 1, 2017

Thank you for your suggestion but it is breaking change, unfortunately. I'm afraid I cannot find valid reason to support the syntax since we can just define it as methods in that case.

I understand your concern for constructor instantiation, however there is also a motivation that would like to define vm data as class properties. I think the current implementation is balanced - predictable and convenient. All properties will be processed as vm data without any exceptions and if you want to add non-reactive properties, you can define it in lifecycle hooks as same as we do with the plain Vue api.

FYI: We cannot inject any value as this into the native ES class constructor. So Super.apply(this, args) will throws if users write their class in native class.

@ktsn
Copy link
Member

ktsn commented Mar 2, 2017

Docs is updated.

@ktsn ktsn closed this as completed Mar 2, 2017
@kimamula
Copy link
Author

kimamula commented Mar 2, 2017

All right, I understand the philosophy of vue-class-component.
Thanks.

@krayush
Copy link

krayush commented Mar 11, 2020

@ktsn : Following up on this.
Still no support of arrow functions inside classes for vue?

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

No branches or pull requests

4 participants