Skip to content

⭐️New: Add rule require-component-name #589

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

Closed

Conversation

privatenumber
Copy link
Contributor

This rule requires components be named

Copy link
Contributor

@armano2 armano2 left a comment

Choose a reason for hiding this comment

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

@hirokiosame Can you add test for its going to prompt about missing name

export default {
  name
}
new Vue({})

Those one are failing

Vue.component('my-component', { })
Vue.mixin({ })

@armano2
Copy link
Contributor

armano2 commented Sep 28, 2018

#265 Additionally we should think what should happens with mixins because for now we have

const componentComments = sourceCode.getAllComments().filter(comment => /@vue\/component/g.test(comment.value))

we should consider adding @vue/mixin? we have no way of telling if something is mixin or component same for Vue.mixin()

@michalsnik
Copy link
Member

Hey, have you looked at the discussion here #265 @hirokiosame ? The PR @chrisvfritz mentioned has been merged and we didn't yet agree whether we want to introduce this rule or not. I'll put on-hold for now and let's continue discussion there, as I have very mixed feelings about it.

@michalsnik michalsnik changed the title Added rule require-component-name ⭐️New: Add rule require-component-name Sep 28, 2018
@privatenumber
Copy link
Contributor Author

Ah great thanks guys. Let me know what you guys decide on and I can work on it further.

@rodrigopedra
Copy link
Contributor

Can we add an option force the component name to match the file name?

e.g.

This is correct::

// src/components/MyComponent.vue

<script>
export default {
    name: 'MyComponent',
}
</script>

<template />

This is incorrect::

// src/components/MyComponent.vue

<script>
export default {
    name: 'MyCompnent', // missed the o
}
</script>

<template />

It would help to catch cases where someone mistyped the component name.

@chrisvfritz
Copy link
Contributor

@rodrigopedra I'd be open to a rule for this, though I think it would mostly only be useful in .jsx files. If you're using .vue files, the only time you should ever need to specify a name for the component should be when the component is recursive.

@rodrigopedra
Copy link
Contributor

Hi @chrisvfritz , thanks for the reply. If possible I wanted to understand the rationale for not specifying a name property for any component. I always set them for consistency but perhaps it is a bit of over engineering on my side.

Anyway, I adapted this PR code to compare the filename to the component's name property, as it was my first time writing a eslint plugin I don't know if it is written the best way.

/**
 * @fileoverview Require components to have names
 * @author Hiroki Osame <[email protected]>
 */
'use strict';

// ------------------------------------------------------------------------------
// Rule Definition
// ------------------------------------------------------------------------------
const utils = require( 'eslint-plugin-vue/lib/utils' );

module.exports = {
    meta : {
        docs    : {
            description : 'require components to have names',
            category    : 'recommended',
            url         : 'https://github.com/vuejs/eslint-plugin-vue/blob/v5.0.0-beta.3/docs/rules/require-component-name.md',
        },
        fixable : null,
        schema  : [],
    },

    create ( context ) {
        return utils.executeOnVueComponent( context, ( object ) => {
            const nameProperty = object.properties.find( ( prop ) => prop.key.name === 'name' );
            const filename     = context.getFilename().replace( /^.*?([a-z0-9_-]+)\.(?:vue|jsx)$/i, '$1' );

            if ( !nameProperty ) {
                context.report( {
                    obj     : object,
                    loc     : object.loc,
                    message : `Expected component to have a name (${filename}).`,
                } );
                return;
            }

            const name = nameProperty.value.value;

            if ( name !== filename ) {
                context.report( {
                    obj     : object,
                    loc     : object.loc,
                    message : `Component name should match filename (${filename} / ${name}).`,
                } );
            }
        } );
    },
};

For using it only on jsx files I can change the regular expression that extracts the filename and apply only for jsx files.

If you can, please review it and I can send a PR or ask @hirokiosame to add it to this PR.

Thanks!

@chrisvfritz
Copy link
Contributor

chrisvfritz commented Nov 6, 2018

If possible I wanted to understand the rationale for not specifying a name property for any component. I always set them for consistency but perhaps it is a bit of over engineering on my side.

@rodrigopedra Great question! 🙂 My reasoning is actually the same as yours: consistency. 😄 When we always use .vue files and don't specify a name, we guarantee that the component's name always matches the file name. When you specify the name again in a name property, you're not only writing more code unnecessarily, but also forcing yourself to either manually make sure the two names match (to avoid confusing behavior) or write some kind of tooling to help you avoid mistakes, like you're doing now. 🙂

If you can, please review it and I can send a PR

Sounds great. This would be a new rule (maybe match-component-file-name), so a new PR would be best. Writing unit tests for the rule will also help us review it, because they'll not only describe the behavior you're trying to create, but if the tests are robust and they pass, we can focus on higher level aspects of the code.

@rodrigopedra
Copy link
Contributor

@chrisvfritz thank you very much for the feedback, I will try to send a PR later this week.

@rodrigopedra
Copy link
Contributor

Hi @hirokiosame , check the work that was done #668 , it was initially based in your code in this PR, and we defaulted to jsx and added options to other file extensions.

@michalsnik
Copy link
Member

Since the #668 have been finally merged to master. Can we close this PR, or is there something we want to do extra here @chrisvfritz ?

@chrisvfritz
Copy link
Contributor

@michalsnik I think this can be closed unless someone objects.

@michalsnik michalsnik closed this Jan 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants