[New] Add `vue/attribute-order` #209

erindepew · 2017-10-09T15:34:34Z

resolves #170

michalsnik · 2018-01-01T21:11:38Z

Hey @erindepew Are you willing to work on this further on? If yes - let us know and please see last comments in #209 for more informations about possible solutions :)

erindepew · 2018-01-01T21:27:25Z

@michalsnik sure! I was just waiting for the community to decide if they want a configuration option and if so what it should be. Are you referring to #170 (comment) ?

michalsnik · 2018-01-01T23:07:01Z

Cool! Sort of, but you know what? Let's start with a simple implementation that does job well according to the official style guide, that is: let's pre-define groups of attributes and only allow people to change order of those groups in configuration, but not individual attributes.
Let's not overthink it just yet.

So the default configuration could look just like this:

'vue/attributes-order': [2, {
  order: [
    'CONDITIONALS',
    'RENDER_MODIFIERS'
    // ...
  ],
}]

Groups should obviously reflect situation from style guide.

After it's done we can iterate and add custom groups support / think about possible edge scenarios etc. WDYT?

erindepew · 2018-01-05T03:01:12Z

@michalsnik I'm going to assume that it makes sense to push off ordering them alphabetically within those groups for a future iteration?

erindepew · 2018-01-05T19:41:06Z

@michalsnik updated, let me know what you think!

michalsnik

Thank you @erindepew for your work :) I added some comments.
@chrisvfritz could you please also take a look from a broader perspective?

michalsnik · 2018-01-06T22:28:29Z

docs/rules/attribute-order.md

+:+1: Examples of **correct** code with custom order`:
+
+```html
+<!-- options: [{ order: ['LIST_RENDERING', 'CONDITIONALS', 'RENDER_MODIFIERS', 'GLOBAL', 'UNIQUE', 'BINDING', 'OTHER_ATTR', 'EVENTS', 'CONTENT', 'DEFINITION'] }] -->


You should use eslint-ready format here, so it's not confusing for users:



michalsnik · 2018-01-06T22:29:41Z

docs/rules/attribute-order.md

@@ -0,0 +1,93 @@
+# enforce ordering of attributes (attribute-order)


I'd go with attributes-order

michalsnik · 2018-01-06T22:31:37Z

docs/rules/attribute-order.md

+
+## :book: Rule Details
+
+This rule aims to enfore ordering of component attributes. The default order is specified in the [Vue styleguide](https://vuejs.org/v2/style-guide/#Element-attribute-order-recommended).


I think you could add default order here as a simple list:

- LIST_RENDERING - CONDITIONALS ...and so on

and then link to official styleguide for more informations. But users should be able to quickly pick up what's going on by just looking at rule documentation :)

I think that we could even give examples for each category, what attributes does it expect actually. I think it would be a huge help!

michalsnik · 2018-01-06T22:38:31Z

lib/rules/attribute-order.js

+function getPosition (attribute, attributeOrder) {
+  const name = attribute.key.name
+  if (name === 'is') {
+    return attributeOrder.indexOf('DEFINITION')


I think you could add function getAttributeType that would return DEFINITION, LIST_RENDERING and so on.

Then you could shorthen this function to:

const attributeType = getAttributeType(attribute.key.name) return attributeOrder.indexOf(attributeType)

This way you'd eliminate multiple attributeOrder.indexOf duplication and make it even more readable :) WDYT?

michalsnik · 2018-01-06T22:40:31Z

lib/rules/attribute-order.js

+module.exports = {
+  meta: {
+    docs: {
+      description: 'enforce ordering of attributes',


What do you think about: enforce order of attributes?

michalsnik · 2018-01-06T22:49:51Z

lib/rules/attribute-order.js

+
+  return utils.defineTemplateBodyVisitor(context, {
+    'VStartTag' () {
+      currentPosition = -1


Theoretically you should also reset previousNode here, though it won't throw error now, because of the fact that previousNode is used after it's redefined in previous conditions of VAttribute

michalsnik · 2018-01-06T22:51:53Z

tests/lib/rules/attribute-order.js

+})
+tester.run('attribute-order', rule, {
+
+  valid: [


Please add some cases with shorthanded bindings

michalsnik · 2018-01-06T22:53:43Z

lib/rules/attribute-order.js

+    context.report({
+      node: node.key,
+      loc: node.loc,
+      message: `Attribute ${currentNode} must go before ${prevNode}.`,


Please add " around attributes. Also I'm thinking whether the message shouldn't be more gentle and say should instead of must

michalsnik · 2018-01-06T23:06:58Z

lib/rules/attribute-order.js

+  } else if (name === 'model') {
+    return attributeOrder.indexOf('BINDING')
+  } else if (name === 'on') {
+    return attributeOrder.indexOf('EVENTS')


Does it catch both v-on and @?

yup! I'm using @click in my tests and it seems to work fine, I'll also add v-on just to be sure

michalsnik · 2018-01-06T23:10:23Z

lib/rules/attribute-order.js

+  } else if (name === 'ref' || name === 'key' || name === 'slot') {
+    return attributeOrder.indexOf('UNIQUE')
+  } else if (name === 'model') {
+    return attributeOrder.indexOf('BINDING')


v-bind:something and :something should also be marked as binding I think, not only v-model.
Also what if someone adds property model="asd"? I think it would mark it as binding and not OTHER_ATTR. You should probably check if attribute.directive is true in all cases where you want to detect v- attribute.

michalsnik · 2018-01-06T23:14:20Z

README.md

@@ -161,7 +161,7 @@ Enforce all the rules in this category, as well as all higher priority rules, wi
 |  | [require-prop-types](./docs/rules/require-prop-types.md) | require type definitions in props |
 | :wrench: | [v-bind-style](./docs/rules/v-bind-style.md) | enforce `v-bind` directive style |
 | :wrench: | [v-on-style](./docs/rules/v-on-style.md) | enforce `v-on` directive style |
-
+| :wrench: | [attribute-order](./docs/rules/attribute-order.md) | enforce ordering of attributes |


Did you add it by hand? I see there is fixable set to null in rule, but I also see 🔧 here. Please run npm run update if you want to update readme or rule documentation header base on rule source.

erindepew · 2018-01-08T15:30:42Z

@michalsnik yup, that all makes sense. Updated!

michalsnik · 2018-01-10T22:54:53Z

Hey @erindepew, I'll take a closer look and test run soon :) So far it's looking very promising 🚀

A little advice for the future though: please try not to rebase during review, as people that check your code after review once again can't see actual changes you performed in order to resolve given suggestions. Rebasing is good right before requesting review or after review :)

michalsnik · 2018-01-28T22:57:59Z

docs/rules/attributes-order.md

+- UNIQUE
+ex: 'ref', 'key', 'slot'
+- BINDING
+ex: 'v-model', 'v-bind'


Does :something="" count here too?

michalsnik · 2018-01-28T22:58:21Z

docs/rules/attributes-order.md

+- OTHER_ATTR
+ex: 'customProp="foo"'
+- EVENTS
+ex: '@click="functionCall"'


Does v-on="" count here too?

michalsnik · 2018-01-28T22:58:57Z

docs/rules/attributes-order.md

+  is="header"
+  v-for="item in items"
+  v-if="!visible"
+  v-once id="uniqueID"


Why there are two attributes in the same line here?

michalsnik · 2018-01-28T23:01:27Z

tests/lib/rules/attributes-order.js

+    },
+    {
+      filename: 'test.vue',
+      code: '<template><div is="header" v-for="item in items" v-if="!visible" v-once id="uniqueID" ref="header" v-model="headerData" myProp="prop" @click="functionCall" v-text="textContent"></div></template>'


You should wrap those lines so it's easier to read :)

chrisvfritz

Thanks for this Erin! I just saw one more thing to update from my perspective. 🙂

chrisvfritz · 2018-01-28T23:04:49Z

lib/rules/attributes-order.js

+    docs: {
+      description: 'enforce order of attributes',
+      category: undefined,
+      recommended: false


This can go in the recommended category and we can remove recommended: false, since we no longer use that property.

erindepew · 2018-02-07T04:23:13Z

Thanks @chrisvfritz and @michalsnik for the review! I made the suggested changes

chrisvfritz

🎉

wadetandy · 2018-02-15T18:45:05Z

This looks great! Would love to get this merged so our team can start taking advantage of it. Anything still outstanding here?

michalsnik

Let's give it a spin! :D

mikesherov · 2018-02-21T21:36:13Z

Congrats @erindepew!

skjnldsv · 2019-04-24T14:37:07Z

Hey! Thanks for this awesome rule @erindepew !

There was a discussion and some comments about making alphabetical order within groups, is this implemented? The documentation does not state anything related 🤔
Thanks :)

erindepew force-pushed the attribute-order branch from 9a5c7b8 to 7dc3e5b Compare October 9, 2017 15:36

erindepew mentioned this pull request Oct 9, 2017

Rule Proposal vue/attribute-order #170

Closed

michalsnik added the work in progress label Oct 15, 2017

mysticatea removed the Ready for review label Dec 2, 2017

michalsnik changed the title ~~vue/attribute-order~~ [New] Add vue/attribute-order Dec 2, 2017

erindepew force-pushed the attribute-order branch 3 times, most recently from 4a71322 to c4112b5 Compare January 5, 2018 19:40

michalsnik suggested changes Jan 6, 2018

View reviewed changes

michalsnik reviewed Jan 6, 2018

View reviewed changes

michalsnik self-assigned this Jan 6, 2018

michalsnik added this to the v4.2.0 milestone Jan 6, 2018

erindepew force-pushed the attribute-order branch from c4112b5 to dfb2b58 Compare January 8, 2018 15:24

attribute order linting

5bc45a4

erindepew force-pushed the attribute-order branch from dfb2b58 to 5bc45a4 Compare January 8, 2018 15:29

michalsnik removed this from the v4.2.0 milestone Jan 28, 2018

michalsnik reviewed Jan 28, 2018

View reviewed changes

chrisvfritz suggested changes Jan 28, 2018

View reviewed changes

updating docs, tests and category

b0b1fad

chrisvfritz approved these changes Feb 7, 2018

View reviewed changes

michalsnik approved these changes Feb 21, 2018

View reviewed changes

michalsnik merged commit abbde6f into vuejs:master Feb 21, 2018

michalsnik removed the work in progress label Feb 21, 2018

ota-meshi mentioned this pull request Feb 22, 2018

Make vue/attribute-order rule fixable #401

Closed

michalsnik pushed a commit that referenced this pull request Feb 24, 2018

New: Add vue/attribute-order rule (#209)

33ce7aa

ElMassimo mentioned this pull request Jul 11, 2018

Add 'OTHER_DIRECTIVE' category to attributes-order #518

Closed

erindepew mentioned this pull request Dec 28, 2019

Rule enhancement: vue/attributes-order #968

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[New] Add `vue/attribute-order` #209

[New] Add `vue/attribute-order` #209

erindepew commented Oct 9, 2017

michalsnik commented Jan 1, 2018

erindepew commented Jan 1, 2018

michalsnik commented Jan 1, 2018 •

edited

Loading

erindepew commented Jan 5, 2018

erindepew commented Jan 5, 2018

michalsnik left a comment

michalsnik Jan 6, 2018

michalsnik Jan 6, 2018

michalsnik Jan 6, 2018

michalsnik Jan 6, 2018

michalsnik Jan 6, 2018

michalsnik Jan 6, 2018

michalsnik Jan 6, 2018

michalsnik Jan 6, 2018

michalsnik Jan 6, 2018

michalsnik Jan 6, 2018

erindepew Jan 8, 2018

michalsnik Jan 6, 2018

michalsnik Jan 6, 2018

erindepew commented Jan 8, 2018

michalsnik commented Jan 10, 2018

michalsnik Jan 28, 2018

michalsnik Jan 28, 2018

michalsnik Jan 28, 2018

michalsnik Jan 28, 2018

chrisvfritz left a comment

chrisvfritz Jan 28, 2018

erindepew commented Feb 7, 2018

chrisvfritz left a comment

wadetandy commented Feb 15, 2018

michalsnik left a comment

mikesherov commented Feb 21, 2018

skjnldsv commented Apr 24, 2019

		@@ -0,0 +1,93 @@
		# enforce ordering of attributes (attribute-order)


		## :book: Rule Details

		This rule aims to enfore ordering of component attributes. The default order is specified in the [Vue styleguide](https://vuejs.org/v2/style-guide/#Element-attribute-order-recommended).

[New] Add vue/attribute-order #209

[New] Add vue/attribute-order #209

Conversation

erindepew commented Oct 9, 2017

michalsnik commented Jan 1, 2018

erindepew commented Jan 1, 2018

michalsnik commented Jan 1, 2018 • edited Loading

erindepew commented Jan 5, 2018

erindepew commented Jan 5, 2018

michalsnik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erindepew commented Jan 8, 2018

michalsnik commented Jan 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisvfritz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erindepew commented Feb 7, 2018

chrisvfritz left a comment

Choose a reason for hiding this comment

wadetandy commented Feb 15, 2018

michalsnik left a comment

Choose a reason for hiding this comment

mikesherov commented Feb 21, 2018

skjnldsv commented Apr 24, 2019

[New] Add `vue/attribute-order` #209

[New] Add `vue/attribute-order` #209

michalsnik commented Jan 1, 2018 •

edited

Loading