Skip to content

Rule proposal: vue/reserved-property-names #144

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

Closed
chrisvfritz opened this issue Aug 9, 2017 · 12 comments
Closed

Rule proposal: vue/reserved-property-names #144

chrisvfritz opened this issue Aug 9, 2017 · 12 comments

Comments

@chrisvfritz
Copy link
Contributor

Please describe what the rule should do:

This rule warns about using potentially reserved property names for props, data, computed properties, and methods. There's an ongoing discussion here, but tentatively, we're thinking of warning for any names that match the regex: /^(\$[^_]|_)/.

We've yet to reach consensus on the exact strategy, but we'd also like to suggest a different naming strategy for private property names used in mixins/plugins. One idea is the $_ prefix, with a suggested namespace like $_pluginOrMixinName_propertyName to avoid conflicts with other mixins/plugins.

What category of rule is this?

[ ] Enforces code style
[x] Warns about a potential error
[x] Suggests an alternate way of doing something
[ ] Other (please specify:)

Provide code examples that this rule will warn about:

A few examples to match:

new Vue({
  data: {
     _foo: ''
  }
})
new Vue({
  data: {
     $foo: ''
  }
})
// In a .vue file
export default {
  data: function () {
    return {
      _foo: ''
    }
  }
} 
// In a .vue file
export default {
  data: function () {
    return {
      $foo: ''
    }
  }
} 
// In a .vue file
export default {
  props: ['_foo']
}
// In a .vue file
export default {
  props: ['$foo']
}
// In a .vue file
export default {
  props: {
    _foo: {}
  }
}
// In a .vue file
export default {
  props: {
    $foo: {}
  }
}
// In a .vue file
export default {
  computed: {
    _foo: function () { ... }
  }
}
// In a .vue file
export default {
  computed: {
    $foo: function () { ... }
  }
}
// In a .vue file
export default {
  methods: {
    _foo: function () { ... }
  }
}
// In a .vue file
export default {
  methods: {
    $foo: function () { ... }
  }
}

A few examples NOT to match:

// In a .vue file
export default {
  data: function () {
    return {
      $_foo: ''
    }
  }
}
// In a .vue file
export default {
  props: ['$_foo']
}
// In a .vue file
export default {
  props: {
    $_foo: {}
  }
}
// In a .vue file
export default {
  computed: {
    $_foo: function () { ... }
  }
}
// In a .vue file
export default {
  methods: {
    $_foo: function () { ... }
  }
}
@armano2
Copy link
Contributor

armano2 commented Aug 9, 2017

@chrisvfritz no-reservered-keys https://github.com/vuejs/eslint-plugin-vue/blob/master/docs/rules/no-reservered-keys.md is doing what you need

i think we should update/add more reserved names, current list is defined here:
https://github.com/vuejs/eslint-plugin-vue/blob/master/lib/utils/vue-reserved.json

@posva
Copy link
Member

posva commented Aug 9, 2017

I think it shouldn't warn on data attributes as they can be defined that way on purpose
@armano2 That list isn't enough because they're not all the internals functions that start with _

@armano2
Copy link
Contributor

armano2 commented Aug 9, 2017

That list isn't enough because they're not all the internals functions that start with _

I know that list is not enough, we should just fill it in with all reserved names, instead of adding new rule.

I think it shouldn't warn on data attributes as they can be defined that way on purpose

In data properties witch starts with _ are invalid and can't be accessed.

@posva
Copy link
Member

posva commented Aug 9, 2017

Adding every name may be too much
yes, they can with $data._foo

@armano2
Copy link
Contributor

armano2 commented Aug 9, 2017

@mysticatea @michalsnik should we extend no-reservered-keys to prevent using names witch match /^(\$[^_]|_)/ or should we create new rule for that?

@chrisvfritz
Copy link
Contributor Author

An issue with lists is we can't predict what might be used by Vue in the future. Using a _ or $ prefix in user code means the app could break at any time, even in a patch release, and possibly even in a dangerously subtle way that doesn't throw an error. It would also be easy for the list to fall out of date.

@mysticatea
Copy link
Member

Thank you for the suggestion.

@chrisvfritz The no-reservered-keys rule has been based on the official API reference. Can I find it in the document that properties which start with _ or $ are reserved at all?

If Vue.js reserves the properties that start with _ or $ of vm instances at all, it should be described in the document, and the rule should warn those.

I found it in the description of data property, but props or others don't warn it.

Properties that start with _ or $ will not be proxied on the Vue instance because they may conflict with Vue’s internal properties and API methods. You will have to access them as vm.$data._property.
https://vuejs.org/v2/api/#data

@posva I think the rule should warn data properties which start with _ (and $) by default. Indeed, it can be accessed by $data._foo, but the fact that {{_foo}} in templates does not work is surprising people (at least, me). Though the option which allows properties which start with _ or $ may be good to have.

@chrisvfritz
Copy link
Contributor Author

@mysticatea No, I haven't added it to the API reference, but may well do in the future. We just need to finalize what our official suggestion is. I'm leaning towards:

  • Never to use _ or $ prefixes for private properties - use $_ instead.
  • Plugins or mixins can use $ prefixes for their public API, using Object.defineProperty:
    Object.defineProperty(Vue.prototype, '$myPublicInstanceProperty', {
      get () { return this.$_pluginOrMixinName_somePrivateProperty }
    })

Perhaps the blacklist is sufficient for $ then, but I think we should still flag all uses of _ prefixes. The thing that really scares me is that a new conflict may arise with a _-prefixed property at any time, even in a patch release, and the resulting bug may even be subtle - or if it produces an error, that error is likely not to be very helpful to users.

@armano2
Copy link
Contributor

armano2 commented Aug 11, 2017

@chrisvfritz i can agree with Never to use _ but there is no downsides from using $ as first parameter with exceptions for reserved list.
In error message we should add suggestion to replace it with $_

I see this rule as:

  • By default only for when property starts with _
  • Optionally add parameter to disallow to starts with /^(\$[^_]|_)/

@michalsnik
Copy link
Member

I think we should stay with the current rule and add only an extra check for properties that starts with _.

@michalsnik michalsnik added this to the v4.1.0 milestone Oct 8, 2017
@chrisvfritz
Copy link
Contributor Author

@michalsnik I think we can hold off on this for now, especially since Vue now warns about the use of reserved properties. Later, once this and the style guide are officially stable, we can possibly make this a separate rule with the style guide's Priority D.

@michalsnik
Copy link
Member

I just created new issue with proposition strictly aligned with the style guide: #246

I think that having one unified way of marking properties/methods as private using universal prefix $_ is a way to go.

@michalsnik michalsnik removed this from the v4.1.0 milestone Nov 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants