-
-
Notifications
You must be signed in to change notification settings - Fork 666
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
feat: add prefer-use-template-ref rule #2554
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution! 🙂
I have a few comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add valid test cases that make sure the rule doesn't error/fail in the following cases:
- A
.vue
file with aref
that is not a template ref. - A
.js
file with aref
.
<template> | ||
<div ref="root"/> | ||
</template> | ||
|
||
<script> | ||
import { ref } from 'vue'; | ||
|
||
const root = ref(); | ||
</script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make the code of the test cases a bit more concise please by removing the empty lines?
fix(fixer) { | ||
return fixer.replaceText( | ||
scriptRef.node, | ||
`useTemplateRef('${scriptRef.ref}')` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fix seems unsafe because it doesn't add any import statements. Adding import
requires complex logic, so could you change it to without auto-fix to keep it simple?
If you want to implement autofix for import
, please add tests for the absence and presence of import
from 'vue'
, tests for different kinds of import
, tests for the two type syntaxes in TypeScript, and tests combining them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or you can change the autofix to a suggestion, so users still can apply it manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could add import
if that's an option but I read in docs that ESLint rules should not apply complex logic. If you're okay with me adding it, I will implement the auto-fix with the added tests, it would definitely help me to have the auto-fix there when going through a bigger codebase to not have to do the change manually many times over and over. But if you prefer the suggestion option, I could also go with that. Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add some tests using the setup()
function and some using the Options API?
<div ref="root" /> | ||
</template> | ||
|
||
<script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is a <script setup>
test, the setup
attribute is required.
<script> | |
<script setup> |
|
||
## :book: Rule Details | ||
|
||
Vue 3.5 introduced a new way of obtaining template refs via |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this rule handle ref
as a function? Can you add documentation for that?
<script setup>
import { ref } from 'vue';
const button = ref();
</script>
<template>
<button :ref="ref => button.value = ref">Content</button>
</template>
Co-authored-by: Flo Edelmann <[email protected]>
Co-authored-by: Flo Edelmann <[email protected]>
Co-authored-by: Flo Edelmann <[email protected]>
Co-authored-by: Flo Edelmann <[email protected]>
resolves #2549