Add rule vue/no-early-return #1889
Conversation
|
@FloEdelmann thanks for your review, I think I've addressed your comments. There is one point however, the one related to Nuxt that I'm not super sure so I'd prefer to leave the rule as it is for now. Perhaps, it's worth opening a new issue specific for that so others can comment on making this rule cover that for Nuxt or not. For me, even covering the |
|
The logic for |
@FloEdelmann ok, if they work the same, I just pushed a small change to cover it (and in a way that if we need to add more properties in the future, it's easily extendable) |
| meta: { | ||
| type: 'problem', | ||
| docs: { | ||
| description: 'disallow early `returns` in `setup` and `data` functions', |
There was a problem hiding this comment.
| description: 'disallow early `returns` in `setup` and `data` functions', | |
| description: 'disallow early `return` in `setup` and `data` functions', |
| @@ -0,0 +1,79 @@ | |||
| /** | |||
| * @author *****your name***** | |||
There was a problem hiding this comment.
Please fill in your name or username here :)
| context.report({ | ||
| node, | ||
| messageId: 'extraReturnStatement', | ||
| data: { functionType: 'data' } |
There was a problem hiding this comment.
It should mention asyncData or data respectively.
| data: { functionType: 'data' } | |
| data: { functionType: property } |
|
|
||
| /** @param {RuleContext} context */ | ||
| create(context) { | ||
| const returnStatementLocations = [] |
There was a problem hiding this comment.
| const returnStatementLocations = [] | |
| /** @type {SourceLocation[]} */ | |
| const returnStatementLocations = [] |
| } | ||
| ] | ||
| }, | ||
| { |
There was a problem hiding this comment.
Please add this test case. I'm afraid it will produce too many errors 🙁
| { | |
| { | |
| filename: 'test.vue', | |
| code: ` | |
| <script> | |
| export default { | |
| setup: () => { | |
| if (maybe) { | |
| return { foo: false } | |
| } | |
| return { foo: true } | |
| }, | |
| data() { | |
| return { | |
| foo: true | |
| } | |
| }, | |
| computed: { | |
| foo() { | |
| return true | |
| } | |
| } | |
| } | |
| </script> | |
| `, | |
| errors: [ | |
| { | |
| messageId: 'extraReturnStatement', | |
| data: { functionType: 'setup' }, | |
| line: 3 | |
| } | |
| ] | |
| }, | |
| { |
| { | ||
| filename: 'test.vue', | ||
| code: ` | ||
| <script> | ||
| export default { | ||
| setup () { | ||
| const foo = computed(() => { | ||
| if (s) { | ||
| return | ||
| } | ||
| }) | ||
| } | ||
| } | ||
| </script> | ||
| ` | ||
| } |
There was a problem hiding this comment.
Unfortunately, this test case fails although it should pass:
https://github.com/vuejs/eslint-plugin-vue/runs/6426396807?check_suite_focus=true#step:5:187
| returnStatementLocations.push(node.loc) | ||
| }, | ||
|
|
||
| onSetupFunctionExit() { |
There was a problem hiding this comment.
Can't the logic for data/asyncData and for setup be shared? I think they should work/behave exactly the same.
| { | ||
| messageId: 'extraReturnStatement', | ||
| data: { functionType: 'data' }, | ||
| line: 3 |
There was a problem hiding this comment.
I think the report should point to line 6.
| </script> | ||
| ` | ||
| } | ||
| ], |
There was a problem hiding this comment.
I found a false positive in the next test source.
<script>
export default {
methods: {
fn () {
if (a) {
return {}; // <- false positive
}
}
},
setup () {
const foo = ref()
return { foo }
}
}
</script>I think we need to check the scope of ReturnStatement correctly.
The implementation of the return-in-emits-validator rule may be helpful.
https://github.com/vuejs/eslint-plugin-vue/blob/master/lib/rules/return-in-emits-validator.js
There was a problem hiding this comment.
Or I think you can use code path analysis like consistent-return rule.
https://eslint.org/docs/developer-guide/code-path-analysis
https://github.com/eslint/eslint/blob/main/lib/rules/consistent-return.js
|
I'll resume this with more time |
fixes #1884