Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

⭐️New: Add vue/component-name-in-template-casing #397

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

Merged

Conversation

Copy link
Member

@ota-meshi ota-meshi commented Feb 19, 2018

This PR adds vue/component-name-in-template-casing rule.
This implements rule proposed in #250

trollepierre, cesasol, niklashigi, samit4me, and aki77 reacted with thumbs up emoji trollepierre and samit4me reacted with laugh emoji trollepierre and samit4me reacted with hooray emoji
michalsnik and others added 9 commits January 28, 2018 23:55
* [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 eslint@3.18.0
* [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 eslint@3.18.0
* [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',
Copy link
Member

@michalsnik michalsnik Feb 21, 2018

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'
Copy link
Member

@michalsnik michalsnik Feb 21, 2018

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({
Copy link
Member

@michalsnik michalsnik Feb 21, 2018

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
Copy link
Member

@michalsnik michalsnik Feb 21, 2018

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 :)

Copy link
Member

@michalsnik michalsnik left a 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`
Copy link
Member Author

@michalsnik
Thank you for review!
I changed it, so please check it.

Copy link
Member

@mysticatea mysticatea left a comment
edited
Loading

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?

Copy link
Member Author

I think we do not have to emphasize html because this rule targets custom components and html elements like <div> and <span> are not targets.
However, I am not good at English, so maybe I haven't understood your intention correctly.

@michalsnik michalsnik self-assigned this Mar 22, 2018
@michalsnik michalsnik changed the title (削除) [New] Add vue/component-name-in-template-casing (削除ここまで) (追記) ⭐️New: Add vue/component-name-in-template-casing (追記ここまで) Apr 1, 2018

## :wrench: Options

Default casing is set to `PascalCase`.
Copy link

@Mouvedia Mouvedia Apr 26, 2018
edited
Loading

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.

Copy link

@samit4me samit4me Jun 22, 2018
edited
Loading

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 🎉

Copy link

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

Copy link

@Mouvedia Mouvedia left a 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'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trollepierre reacted with thumbs up emoji
Copy link
Contributor

Any updates on this pull request, @ota-meshi? I would love to see this merged!

trollepierre and samit4me reacted with thumbs up emoji samit4me reacted with laugh emoji samit4me reacted with hooray emoji

Copy link
Member

michalsnik commented Jul 11, 2018
edited
Loading

Sorry guys for such a long absence..

I think that:

  • component-name-in-template-casing is a good name
  • PascalCase is sensible default aligned with our style guide

Also:

  • this rule checks only content of SFC's <template> tags
  • this rule looks only for custom elements, using this helper thus it's exactly targeting what name suggests: component-name

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 unassigned category for some time, and it would give as a moment to test it in real life projects.

samit4me, ota-meshi, trollepierre, and niklashigi reacted with thumbs up emoji

Copy link
Member

@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 :)

chrisvfritz and ota-meshi reacted with thumbs up emoji

Copy link
Member Author

@michalsnik I agree.
If few custom elements are used, I think that it can be solved with option.
I think that it is unlikely that products using Vue.js uses a lot of custom elements, so I think that this rule will be useful.

@michalsnik michalsnik merged commit c49a2e2 into vuejs:master Jul 30, 2018
@ota-meshi ota-meshi deleted the add-component-name-in-template-casing branch July 30, 2018 13:28
Copy link

stevenadams commented Sep 17, 2018
edited
Loading

@ota-meshi @michalsnik - any reason this rule is still left in undefined category? According to the style guide it should be 'strongly-recommended'

https://github.com/vuejs/eslint-plugin-vue/blob/master/lib/rules/component-name-in-template-casing.js#L25

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

trollepierre reacted with thumbs up emoji

Copy link

lfaoro commented Dec 3, 2018

This creates an issue with Vuetify, it'll transform all Vuetify components from e.g. <v-app> -> <VApp>

TitanFighter and squalsoft reacted with thumbs up emoji

Copy link

zifnab87 commented Dec 3, 2018

It also creates problems with bootstrap-vue. e.g <b-row> --> <BRow>

TitanFighter reacted with thumbs up emoji

Copy link

lfaoro commented Dec 6, 2018

as a quick workaround turn it off in the .eslintrc.js

 rules: {
 "vue/component-name-in-template-casing": "off"
 },
trollepierre reacted with thumbs up emoji

Copy link
Member Author

I did not know that it is must use kebab-case when defining components with kebab-case.
https://vuejs.org/v2/guide/components-registration.html

This rule reports false positives.

I think that it is necessary to limit the verification target to those registered within the component.
I will fix this rule.

Copy link

adrlen commented Dec 6, 2018

I also have those warnings :

warning: Component name "router-link" is not PascalCase (vue/component-name-in-template-casing)
warning: Component name "transition-group" is not PascalCase (vue/component-name-in-template-casing)
etc

With Vue native components.

Copy link
Member Author

ota-meshi commented Dec 7, 2018
edited
Loading

@adrlen Hello!
bootstrap-vue seems not to be able to use PascalCase, but I think that router-link and transition-group can also be used in PascalCase.

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

Copy link
Member Author

I might not notice this thread comments so please open the new issue.

Copy link

adrlen commented Dec 7, 2018

Hi @ota-meshi, thanks for your response !

It works indeed with Transition, TransitionGroup, but I have this error when using RouterView and RouterLink :

[Vue warn]: Unknown custom element: <RouterView> - did you register the component correctly? For recursive components, make sure to provide the "name" option.

Copy link
Member Author

ota-meshi commented Dec 7, 2018
edited
Loading

@adrlen
I don't know the cause it does not work 🤔
Are you installing and using the vue-router package?

https://www.npmjs.com/package/vue-router

Copy link

adrlen commented Dec 7, 2018

https://github.com/vuejs/vue-router/releases/tag/v3.0.2

I was using 3.0.1 ;)

Thanks again.

Copy link
Member Author

adrlen reacted with thumbs up emoji

Copy link

lfaoro commented Dec 12, 2018

Still, this rule should be removed from the defaults

Shamus03 reacted with thumbs up emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
4 more reviewers

@samit4me samit4me samit4me left review comments

@mysticatea mysticatea mysticatea requested changes

@Mouvedia Mouvedia Mouvedia approved these changes

@michalsnik michalsnik michalsnik approved these changes

Reviewers whose approvals may not affect merge requirements
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /