Add option to show warnings when props could be accessed via this #2101
Conversation
Ref vuejs#1636, this is my first stab at adding an option to not treat generic access to `this` as 'using' properties / data / computed, etc. If this is the right approach, I can try adding tests if needed. It's more general I think than what you'd proposed in that issue, but for my use case it works well.
|
Thank you for this PR. |
|
OK, before I go too far, can I check the original issue makes sense to you (it was a while ago, but I just got some time to revisit)? What I currently see (and am still to be convinced this is not the wrong behaviour) is that the plugin considers all elements in this code as 'used' and shows no warnings: <script>
export default {
computed: {
life () {
return 42
}
},
methods: {
getThis () {
return this
},
}
}
</script>My understanding is that is because returning I tried to follow your example in #1636 whereby the option If that all makes sense, I will write this up in docs, and add a test case—but only if you still think this is the right way to go... |
|
Thanks for the explanation. But I'm still not sure. I need the specific source code. I don't think the current changes match what you want to do. |
I'm not sure what you mean by this? I've set this up locally with my changes, and by putting module.exports = {
extends: [
// add more generic rulesets here, such as:
// 'eslint:recommended',
// 'plugin:vue/vue3-recommended',
'plugin:vue/recommended' // Use this if you are using Vue.js 2.x.
],
rules: {
// override/add rules settings here, such as:
// 'vue/no-unused-vars': 'error'
'vue/no-unused-properties': ['warn', {
'groups': [ 'props', 'data', 'computed', 'methods' ],
'deepData': true,
'ignorePublicMembers': true,
'stopReporting': []
}],
}
}and in Sublimetext, I now see this: |
|
OK, I think I see what you mean. The change is too general at the moment, and also still warns about explicit accesses such as |
|
Right, I think part of the solution is in case 'MemberExpression': {
if (parent.object === node) {
// `arg.foo`
const name = utils.getStaticPropertyName(parent)
return name
? new PropertyReferencesForMember(parent, name, withInTemplate)
: ANY // ←----- This line should return NEVER to avoid suppressing warnings?
}
return NEVER
}With However, I can't see how to access the configuration options within that utility function, to selectively return |
|
I added the logic into what I think is the right place. Please see if this is closer to what you had in mind with the options. |
|
I'm still not quite sure what this PR change is trying to do. Could you add test cases and documentation to clarify what you want to do? |
|
I will try, but I'm not sure just documenting the new option is enough—especially since it's just an implementation of your own suggestion... Perhaps this example is clearer? Take this code—which of the <script>
export default {
computed: {
one () {
return 1
},
two () {
return 2
},
three () {
return 3
},
},
methods: {
handler () {
console.log(this.one)
const i = 'two' // Could be two or something else
console.log(this[i])
return this
},
}
}
</script>The current behaviour of the plugin is to warn about none of them being unused, despite only If that makes it clear what I'm trying to achieve, I'm happy to add test cases and try to document that. But if it's still not clear, I'm not sure how else to describe the issue... |
|
Does this change make sense now? I will try to update the documentation to explain if so, but want to make sure you're ok with it. |
|
I am very busy right now with my daughters having the flu. So I can't check this change yet. |
|
Sorry to hear that—no problem at all, just checking in. |
|
Thank you for adding the test cases. But I think the PR change still doesn't handle the new options well. {
filename: 'test.vue',
code: `
<script>
export default {
data () {
return {
foo: {
bar: 1
},
baz: 2
}
},
methods: {
handler () {
console.log(this.baz)
return this.foo
},
}
}
</script>
`,
options: [{
groups: ['data'],
deepData: true,
stopReporting: []
}],
errors: [
{
message: "'foo.bar' of data found, but never used.",
line: 7,
}
]
}To handle them better, I think we should pass an option to |
|
Yes, that's what I concluded in this comment, but I wasn't sure if passing options down there was even possible let alone desirable. When I get a bit more time, I'll try to update the PR. |
|
OK, so one clarification; the In fact, looking closer, I'm not sure exactly how to pass the options down, without the ENUM to test against, or by converting to booleans for each option ( |
|
OK, I've had a go at passing the options in. I'm not sure it's the most elegant way, and the defaults ( Also, that utility function is called from |
|
My latest changes do work as expected here I think now—I've added an additional test. The new options do not mention the word 'this' any more. If you're happy with the changes as they are, I'll write up in the documentation. |
There was a problem hiding this comment.
The tests on CircleCI are failing at an unrelated file, maybe they are just flaky @ota-meshi.
Other than that, this looks good to me 🙂
Ref #1636, this is my first stab at adding an option to not treat generic access to
thisas 'using' properties / data / computed, etc. If this is the right approach, I can try adding tests if needed. It's more general I think than what you'd proposed in that issue, but for my use case it works well.