-
Notifications
You must be signed in to change notification settings - Fork 7
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 linting rules for Vue components #70
Conversation
Ping @yokuze |
b99a6c7
to
3fc1ce7
Compare
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.
@crgwbr thanks for doing all of this! A few questions / comments.
'shouldMatchCase': true, | ||
}, | ||
], | ||
// 'vue/new-line-between-multi-line-property': 'off', |
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.
Why are these lines commented out?
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.
It's been long enough that I don't remember for sure. But I think my purpose with the commented out lines was to note that we've considered the rule and chosen not to turn it on (vs. a rule not being listed at all, meaning we haven't thought about it yet).
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.
It's been long enough that I don't remember for sure. But I think my purpose with the commented out lines was to note that we've considered the rule and chosen not to turn it on (vs. a rule not being listed at all, meaning we haven't thought about it yet).
// 'vue/no-restricted-static-attribute': 'off', | ||
// 'vue/no-restricted-v-bind': 'off', | ||
// 'vue/no-static-inline-styles': 'off', | ||
// 'vue/no-template-target-blank': 'TODO', |
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 open a ticket for us to address the TODO rules here?
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.
Done: #72
env: { | ||
browser: true, | ||
}, | ||
rules: Object.assign( |
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 ran these rules against one of our projects and I'm seeing these errors for every file:
1:1 error Definition for rule 'vue/no-deprecated-v-is' was not found vue/no-deprecated-v-is
1:1 error Definition for rule 'vue/no-invalid-model-keys' was not found vue/no-invalid-model-keys
1:1 error Definition for rule 'vue/no-this-in-before-route-enter' was not found vue/no-this-in-before-route-enter
1:1 error Definition for rule 'vue/require-emit-validator' was not found vue/require-emit-validator
All of the other rules seem to be executing fine. Any idea why? FWIW, I used npm link
to test so maybe that's causing a problem somehow?
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 we disable the vue/no-multiple-template-root
rule? I think it's "on" by default, but multiple template roots are a new feature in Vue 3, are there are legitimate use cases for it.
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.
All of the other rules seem to be executing fine. Any idea why? FWIW, I used npm link to test so maybe that's causing a problem somehow?
Yeah, I remember having issues testing this with npm link
. I think it's related to differing versions of eslint
/ eslint-plugin-vue
that' only resolve correctly with a real install. If we can get this merged and cut a beta release, I'll work on a MR to test using it.
Can we disable the vue/no-multiple-template-root rule? I think it's "on" by default, but multiple template roots are a new feature in Vue 3, are there are legitimate use cases for it.
✅
3fc1ce7
to
6af171d
Compare
^ Rebased from master to fix merge conflicts. |
6af171d
to
2a18ccb
Compare
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 think this is valuable enough as-is (before addressing the TODOs) for us to cut a beta and let @crgwbr test it out. We need to get something integrated before our Vue codebases continue to grow too much.
No description provided.