-
-
Notifications
You must be signed in to change notification settings - Fork 678
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
⭐️New: Add vue/component-name-in-template-casing
#397
⭐️New: Add vue/component-name-in-template-casing
#397
Conversation
* [Update] Make `vue/max-attributes-per-line` fixable * [fix] bug and style * [fix] Switch indent calculation method with node and attribute * [fix] don't handle indentation * [add] autofix test max-attributes-per-line.js
* [Update] Make `vue/order-in-components` fixable This Commit makes `vue/order-in-components` fixable. In case of `The "A" property should be above the "B" property` error, autofix will move A before B * [fix] fail test at [email protected] * [fix] If there is a possibility of a side effect, don't autofix * [fix] failed test at node v4 * [update] use Traverser * [fix] failed test [email protected] * [fix] I used `output: null` to specify "not fix"
meta: { | ||
docs: { | ||
description: 'enforce specific casing for the component naming style in template', | ||
category: 'strongly-recommended', |
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 set category
to undefined, as it would require us to do major release. And we're not going to do so in the next 2 weeks. We're focusing on minor changes and will do a major release once per 2-3 months probably to avoid confusion.
|
||
create (context) { | ||
const options = context.options[0] | ||
const caseType = allowedCaseOptions.indexOf(options) !== -1 ? options : 'PascalCase' |
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.
You could add const defaultCase = 'PascalCase'
right below allowedCaseOptions
, and just use it here. It would be more obv I guess :)
if (casingName !== name) { | ||
const startTag = node.startTag | ||
const open = tokens.getFirstToken(startTag) | ||
context.report({ |
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 empty line above, for clarity
message: 'Component name "{{name}}" is not {{caseType}}.', | ||
data: { | ||
name, | ||
caseType: caseType |
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.
You can also use shorthand 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.
Thank you @ota-meshi. Great work :) I have only few simple suggestions
* Default case to constant. and used it. `const defaultCase = 'PascalCase'` * `'category'` to `undefined` * Add empty line for clarity * I used object shorthand. `caseType: caseType` to `caseType`
@michalsnik |
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.
Almost LGTM, great.
But I'm thinking if the name of this rule needs html-
prefix because this rule depends on HTML-specific syntax. 🤔
Maybe html-element-name-casing
or something like?
I think we do not have to emphasize html because this rule targets |
vue/component-name-in-template-casing
vue/component-name-in-template-casing
|
||
## :wrench: Options | ||
|
||
Default casing is set to `PascalCase`. |
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 shouldn't be the default for the reasons Iv listed on #250 (comment)
Please don't merge this until that's fixed.
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.
From my understanding, we can only lint Single File Components and according to the Vue Style Guide PascalCase is strongly recommended in SFC's, so this seems like a sensible default. If you want to use kebab-case it's configurable so it can be either. +1 for this PR, really looking forward to it being merged 🎉
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 looks like this plugin uses the Style Guide as the source of truth, so If you believe kebab-case should be the default (a lot of people I work with agree) it might be more appropriate to raise a new issue over on the Style Guide repo itself, see https://github.com/vuejs/vuejs.org/blob/e93b8371d73e4467dd8152703ddf1db423f489a2/src/v2/style-guide/index.md#single-file-component-filename-casing-strongly-recommended
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.
kebab-case should be the default
const casing = require('../utils/casing') | ||
|
||
const allowedCaseOptions = ['PascalCase', 'kebab-case'] | ||
const defaultCase = 'PascalCase' |
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.
Any updates on this pull request, @ota-meshi? I would love to see this merged! |
Sorry guys for such a long absence.. I think that:
Also:
Given that I think it's ready to be merged. Unless I missed some vital information in all above comments? It would still be in an |
@ota-meshi I was speaking with @chrisvfritz and we think that it might make sense to add extra option to allow whitelisting custom components names, so that it's possible to use non-vue components even with this rule enabled in ESLint. I'm happy to hear your thoughts too :) |
…sing # Conflicts: # README.md
@michalsnik I agree. |
@ota-meshi @michalsnik - any reason this rule is still left in undefined category? According to the style guide it should be 'strongly-recommended' I put in a real quick PR for this as I know the team prob have bigger things to work on, hope that's ok :) - #577 |
This creates an issue with Vuetify, it'll transform all Vuetify components from e.g. |
It also creates problems with bootstrap-vue. e.g |
as a quick workaround turn it off in the
|
I did not know that it is must use This rule reports false positives. I think that it is necessary to limit the verification target to those registered within the component. |
I also have those warnings :
With Vue native components. |
@adrlen Hello! I confirmed that it works with the following code. <RouterView />
<TransitionGroup name="list" tag="p">
<span v-for="item in items" :key="item" class="list-item">
{{ item }}
</span>
</TransitionGroup> If you prefer kebab-case you can also change the option. "vue/component-name-in-template-casing": ["error", "kebab-case"] https://vuejs.github.io/eslint-plugin-vue/rules/component-name-in-template-casing.html#options |
I might not notice this thread comments so please open the new issue. |
Hi @ota-meshi, thanks for your response ! It works indeed with
|
@adrlen |
https://github.com/vuejs/vue-router/releases/tag/v3.0.2 I was using 3.0.1 ;) Thanks again. |
@adrlen Thank you for the information! It seems that PascalCase has become available for v3.0.2. 😄 https://github.com/vuejs/vue-router/blob/v3.0.2/src/install.js#L46 |
Still, this rule should be removed from the defaults |
This PR adds
vue/component-name-in-template-casing
rule.This implements rule proposed in #250