Skip to content
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

Rule proposal: require-component-name #265

Closed
danielbastos11 opened this issue Nov 30, 2017 · 26 comments
Closed

Rule proposal: require-component-name #265

danielbastos11 opened this issue Nov 30, 2017 · 26 comments

Comments

@danielbastos11
Copy link

danielbastos11 commented Nov 30, 2017

Please describe what the rule should do:

This rule warns you if the component's name property is undefined

What category of rule is this?

[x] Enforces code style
[ ] Warns about a potential error
[ ] Suggests an alternate way of doing something
[ ] Other (please specify:)

Provide 2-3 code examples that this rule will warn about:

// bad
export default {
  render(){
    return <div />
  }
}
// good
export default {
  name: 'div-component',
  render(){
    return <div />
  }
}

Why should this rule be included

Naming components is a good practice. It makes it easier to debug your code with Vue Devtools, and helps you to keep your code organized. So much so that at least 2 rules on the Style Guide address the issue of how to name your components - one of them is a essential rule.

@michalsnik
Copy link
Member

Hey, thanks for posting this proposal. I think this might even be something worth to be mentioned in Styleguide itself / cc @chrisvfritz

@chrisvfritz
Copy link
Contributor

Hmm, I would probably leave this out of the style guide and keep it in the uncategorized section here, because a name option is not actually useful in the vast majority of cases. For example:

  • When registering a component locally, as the Vue devtools will use the component's local name.
  • When registering a component globally, as providing a name is part of that process.
  • When importing a .vue component to be used in a render function, as a name will be derived from the __file property that's added to the default export.

So now that I think about it actually, and especially since functional components can now be written in .vue files, I can't think of a real use case where a component needs the name option to be specified. Did I miss any common (or even uncommon) scenarios?

@LinusBorg
Copy link
Member

The name is necessary when rendering a component recursively.

@chrisvfritz
Copy link
Contributor

Yes, that's true! I think a rule to warn about recursive components without a name would be useful. I'd put that in strongly-recommended, but probably leave it out of the style guide still because it's a bit niche.

@danielbastos11
Copy link
Author

danielbastos11 commented Dec 4, 2017

My use case is slightly different.
I'm using babel-plugin-transform-vue-jsx, which makes my life extremely easier by allowing me not to register components locally and providing me with a Vue.js syntax that looks almost exactly like React (this is really important in my use case)

This is a extremely simplified version of one my files:

import Table from '../components/Table'

export default {
  name: 'SpList',
  props: {
    visits: { type: Array, default: [] }
  },
  render() {
    let { visits } = this
    return (
      <div>
        <Table
          header={['Location', 'Date', 'Users', 'Status']}
          items={visits} factory={rowFactory}
        />
      </div>
    )
  }
}

Notice how I never registered Table as a local component. This leads Vue devtools to display my component as "AnonymousComponent".

While I have to say that I probably said "my use case" too many times to make this proposal global to the Vue community, I think it is importante enough to people using babel-plugin-transform-vue-jsx (which is probably a lot of people!)

@chrisvfritz
Copy link
Contributor

@danielbastos11 The problem isn't actually that that you're using JSX, but rather that you're not using a .vue file. Even if you're not using a template, I still recommend a .vue file for this reason, and because you may also want add scoped styles later. For example, if you're a fan of CSS modules:

<script>
import Table from '../components/Table'

export default {
  name: 'SpList',
  props: {
    visits: { type: Array, default: [] }
  },
  render() {
    let { visits, $style } = this
    return (
      <div class={$style.tableContainer}>
        <Table
          header={['Location', 'Date', 'Users', 'Status']}
          items={visits} factory={rowFactory}
        />
      </div>
    )
  }
}
</script>

<style lang="scss" module>
.tableContainer {
  /* ... */
}
</style>

I guess maybe a rule that's missing from the style guide is to use .vue files wherever possible. 😄

@LinusBorg
Copy link
Member

LinusBorg commented Dec 4, 2017

@chrisvfritz I agree that using .vue files is preferable, but consider this scenario:

If I use a component someone published, a file that was bundled with rollup or webpck - that component will not have a name property, not a __filename (added only in dev mode), right?

And if I use it like @danielbastos11 does, I will end up with an anonymous component in the devtools - so I'm forced to register it locally just to get a name or manually add a name to it.

The same woudl happen if I passed such dynamically to <component :is="myComponent">, by the way. Sure,. I could register it locally and then do <component :is="$components.myComponent">, but now the template expression is longer than it needs to be, to force a name.

...or am I missing something?

@chrisvfritz
Copy link
Contributor

@LinusBorg If __filename is only added in dev mode, then it sounds like 3rd-party component libraries would be an exception and a name property should be required in those codebases. 👍

This also makes me wonder if there should be a separate rules category (and maybe section of the style guide or completely separate page) for 3rd-party component libraries.

@danielbastos11
Copy link
Author

@chrisvfritz Thanks a lot for your feedback!

I don't think a rule recommending the use of .vue is desirable, though.
I mean, I think it's pretty reasonable to assert that one of Vue's main goal is to allow for maximum flexibility - for instance, you can use Vue with render functions or templates, and it works great with both. Recommending Single Files Components over plain old JSX files would kill some of this flexibility, in my opinion.

@danielbastos11
Copy link
Author

BTW, I'd agree that adding names to a library's component should be the library's author concern. If the 3rd-party components I'm using aren't named, I should either make a PR or live with it.

@chrisvfritz
Copy link
Contributor

chrisvfritz commented Dec 5, 2017

@danielbastos11 I might be misunderstanding, but how would the recommendation for .vue files be less flexible? If you're using JSX, you're already guaranteed to be using a build process, and they don't restrict your options in any way I'm aware of.

Also, "plain old JSX files" are already a special kind of file that requires preprocessing.

@danielbastos11
Copy link
Author

@chrisvfritz I meant that by not making .vue an official style guide recommendation, we'd make life easier to people who don't want to include .vue on their build process, since they wouldn't need to adhere to this rule.

I know these people could just turn the rule off on their .eslintrc, but I've seen many great component libraries just assume everybody is using .vue and not bundle their assets at all. I'm afraid such recommendation might make this pattern even more widespread in the community.

@danielbastos11
Copy link
Author

Would everybody be ok with enforcing naming components created on .jsx files?
I think this should be a Priority B rule, but any other category would work.

@chrisvfritz
Copy link
Contributor

we'd make life easier to people who don't want to include .vue on their build process

I'm still not convinced by this, as the whole point of a style guide is to give people rules to follow that will unequivocally make their life easier. Choosing not to use .vue files when you already have a build process has no benefits and only creates more work, so I'm not sure why you'd want the option. 😕

I agree that a name option should exist on .js file components that do not contain JSX, as they might not necessarily be using a build system, even if they're using ES2015+. But once you introduce JSX, which will always require that build step, there's just no reason I can think of not to use .vue files.

If I could be provided with a potential downside, I'd be open to the rule, but otherwise I fear it would imply a recommendation for a pattern I don't see a valid use case for.

@danielbastos11
Copy link
Author

But once you introduce JSX, which will always require that build step, there's just no reason I can think of not to use .vue files.

I guess I just see Single File Components and JSX as two completely independent features, and I don't see the point in tying them up together like that.

Anyway, I think this rule will be a valuable contribution to the community even if limited to .js files without JSX. I'm open to work on a PR if we agree on those terms.

@chrisvfritz
Copy link
Contributor

That sounds good to me. 🙂 I agree it should be in strongly-recommended. Not sure if @michalsnik already has a good way of detecting non-SFC component files, but checking if the file exports an object that with a template or render property might work.

@michalsnik
Copy link
Member

Currently we don't detect non-SFC, we detect all Vue components, both SFC and nonSFC. Theoretically we could check if the file has .vue extension or not, but there might be edge cases like you mentioned, with no template tag, so checking components' properties might do the trick.
We could add another method in our utils: https://github.com/vuejs/eslint-plugin-vue/blob/master/lib/utils/index.js#L379 called isNonSFCVueComponent and corresponding extecuteOnNonSFCVueComponent.

@robcresswell
Copy link

Another potential use case for this rule is that vue-test-utils detects component presense by name IIRC. Also fwiw, I don't think we gain anything by making the plugin only enforce style guide rules? This could easily sit under an "extras" rule for people who want to use it; something like "These rules are not part of the style guide, but my be optionally enforced in your project if you choose". It seems a little excessive to have an entirely separate plugin for that.

@chrisvfritz
Copy link
Contributor

@robcresswell We actually have an "Uncategorized" section for rules not in the style guide. 🙂

@eddyerburgh
Copy link
Member

eddyerburgh commented Jun 9, 2018

@robcresswell vue-test-utils defaults to use the constructor to identify component instances, so there's no requirement for the component to have a name

@robcresswell
Copy link

@eddyerburgh Ah, I'm unsure where I had that bit of info stored from. Thanks for clarifying.

@robcresswell
Copy link

@chrisvfritz So you do! Well, anyway, I'll leave it as "I'm rather a fan of this rule".

@chrisvfritz
Copy link
Contributor

Just following up with an update. Once this vue-loader PR is merged, a name property should truly only be required for recursive components when using .vue files. Even pre-compiled 3rd-party components will be able to have the name inferred. 🙂

So then I see two possible uncategorized rules that are not mutually exclusive:

  • require-sfcs: Whenever a Vue component definition is exported from a non-.vue file, provide a warning like:

    Single-file (.vue) components (SFCs) should be used for all components, even when only containing JavaScript. SFCs provide extra context to Vue's devtools, warnings, and editor extensions, allowing the best possible development experience. They also reduce the effort of switching between templates and render functions, or adding styles to a previously unstyled component.

    Note that I bolded "exported", because module.exports or export default will mean they're using a module system, which means they're using a build system that supports .vue files. 🙂 If a user is assigning components to variables within a single file or globally registering with Vue.component in separate files, it will either mean they're likely not using a build system or they're using a very simplistic one that just concatenates files of the same type.

    With the latest updates, the only reason I don't want to add this to strongly-recommended is I don't feel 100% confident there aren't edge cases I'm missing where using .vue files would be difficult or impossible, despite the use of a module system.

  • require-name-on-non-sfcs: This covers users who are defining components in .js(x) files and want to continue doing so, despite the fact that I've still only seen disadvantages. 😜 It could catch both Vue component definitions that are exported or assigned to a variable, both of which mean the component definition could be used directly in a render function instead of ever being registered.

What are thoughts on these?

@sodatea
Copy link
Member

sodatea commented Dec 25, 2018

@chrisvfritz I personally don't like this feature to be added in vue-loader, as the .options.__file field is only useful for component libraries but when adding to vue-loader it unnecessarily bloats all users' production code.
Even for component libraries, many prefer custom name than the inferred __file property.
A common scenario is that a button component may located at button/index.vue or button.vue but named as md-button. In the first case, the inferred name is index, which only adds confusion to users. In the second case, does it really improve the debug experience?

@chrisvfritz
Copy link
Contributor

@sodatea I agree a vue-loader option to prevent adding __file makes sense. I still think it should be on by default though, because the small amount of extra code is negligible compared to the improved debugging experience for components without a name. And in Vue CLI, we can automatically disable it in production when --target is app (default).

In the cases you provided though, a user can provide a name property and it will be used instead of __file for an improved debugging experience. However, I think what would be a much better experience is a vue-loader option to make implicit names relative to a set of provided folder names (e.g. ['component', 'views']). So src/components/dashboard/button/index.vue would get the implied name DashboardButton. To make this work, we'd want to add another property __name that newer versions of Vue and the devtools could prefer.

As a side note though, I discourage people from using the component organization strategies you provided as examples. The reason is the vast majority of navigation in most projects tends to use fast file switching in editors, which this strategy works very poorly with. In those dropdowns, the filename is typically front and center, while the path is grayed out and takes longer to see. With all generic components in on flat folder, it's also takes only a second or two to bring up lists of related components, including their hierarchy.

Does that make sense?

@ota-meshi
Copy link
Member

The require-name-property rule has already been added (#945), and you can use the rule to find missing name. So I will close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants