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

New rule order-in-components #42

Merged
merged 11 commits into from
Jun 26, 2017
Merged

Conversation

michalsnik
Copy link
Member

@michalsnik michalsnik commented Jun 21, 2017

This PR adds new rule regarding this proposition: #13

This rule will be applied on:

  • export default {} inside .vue and .jsx files
  • new Vue({}) in any file
  • Vue.component('xxx', {}) or component('xxx', {}) in any file

Thing to be noted - we won't support dynamically loaded components, as it's not so obvious to tell if it actually is or is not a vue component.

CallExpression (node) {
// Vue.component('xxx', {}) || component('xxx', {})
if (!isVueComponent(node)) return
checkOrder(node.arguments.slice(-1)[0].properties, orderMap, context)
Copy link
Member

Choose a reason for hiding this comment

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

I guess that this line can throw TypeError if component() with no argument exists.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, but I couldn't imagine such a component though. Fixed it anyway :)

Copy link
Member

Choose a reason for hiding this comment

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

Many people use ESLint in editor integrations. In that case, ESLint runs on every character typing, so TypeErrors mess development experience. 😉

NewExpression (node) {
// new Vue({})
if (!isVueInstance(node)) return
checkOrder(node.arguments[0].properties, orderMap, context)
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

ExportDefaultDeclaration (node) {
// export default {} in .vue || .jsx
if (!isComponentFile(filePath)) return
checkOrder(node.declaration.properties, orderMap, context)
Copy link
Member

Choose a reason for hiding this comment

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

I guess that this line can cause TypeError if default exports without object literals (e.g. export default 777) exists.

Copy link
Member Author

Choose a reason for hiding this comment

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

Very good point! Fixed :)

Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

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

LGTM.

docs: {
description: 'Keep order of properties in components',
category: 'Best Practices',
recommended: true
Copy link
Member

Choose a reason for hiding this comment

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

I think that it needs major version if we keep recommended: true.

Copy link
Member Author

@michalsnik michalsnik Jun 26, 2017

Choose a reason for hiding this comment

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

yeah, I was thinking the same. So maybe we'll wait with this until other PR's are merged, and then we'll bump to 4.0.0. If it will be quite stable - then we could finally release it without the beta tag.

@michalsnik
Copy link
Member Author

I removed recommended setting and will release it as minor atm. We'll update this setting in a while @mysticatea

@michalsnik michalsnik merged commit 9fff64d into master Jun 26, 2017
@michalsnik michalsnik deleted the new-rule-order-in-components branch June 26, 2017 21:00
@privatenumber
Copy link
Contributor

@michalsnik Will this eventually have support for components that were assigned to a variable, and then exported? eg.

const ComponentName = {
     name: 'ComponentName',
     render(h) { return h('div'); }
};

export default ComponentName;

@privatenumber
Copy link
Contributor

@michalsnik

@michalsnik
Copy link
Member Author

I'm not sure yet @hirokiosame. Theoretically we could support this case, and one when you only use that component in current template, but don't even export it. But this needs further investigation, so feel free to create a proposition as separate issue.

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.

3 participants