[Fixes #293] Update indent rule

[michalsnik]

As pointed in #293 currently indent rule requires attributes to be aligned as the first one:

<div id="foo"
     class="abc"
     style="font-color: red;"
>
</div>

It's not that common I think, and most people prefer to use it like this:

<div id="foo"
  class="abc"
  style="font-color: red;"
>
</div>

This PR updates indent rule to force the second version.

Though I'm thinking whether we shouldn't add an optional setting to make it possible to align to the first token if needed.

What do you think guys? @mysticatea @chrisvfritz

[mysticatea]

Hmm, I think it's enough common style that it aligns rest attributes to the first attribute; it exists in the source code of this PR page as well (though both styles are mixed).

[chrisvfritz]

I personally don't see a use case for either of them. 😄 They seem just as strange as these styles in JavaScript:

const myObject = { foo: 'a',
                   bar: 'b',
                   baz: 'c',
}

const myObject = { foo: 'a',
  bar: 'b',
  baz: 'c',
}

I don't think ESLint even provides an option to format objects with either of these styles.

That said, @mysticatea is right that these are both unfortunately quite common in HTML. I believe at least one popular editor (don't remember which one) even auto-formats HTML in the first style by default. So an optional setting may be the best way to go, defaulting to the current behavior for backwards-compatibility.

[michalsnik]

Well, it's not a JS, so it might differ after all :D I'm fine with an optional setting, but would argue that the second version should be a default one, and aligned to first argument as optional formatting preference. This rule is fixable, so it shouldn't be a big problem @chrisvfritz

Also we have another rule that doesn't allow attributes in first line in case there are other attributes in the following lines (max-attributes-per-line) so we don't have to worry too much about this kind of change I think, especially when the official preferred way of writing components doesn't approve above scenarios anyway :)

[chrisvfritz]

I'm not strongly opinionated in which of these is the default. I think it's possible that the current (aligned) style is more popular, but I find your proposed change slightly less offensive for multi-line. 😄 I can see arguments either way.

On a technical note, since this is fixable: how do we know the specific indentation level to use if we can't tell from context?

[michalsnik]

I'll ask on discord to see how people think about it :)

less offensive alignment is the argument I was looking for 🙌
Especially if you already have one attribute in first line and others aligned to it, and then you decide to actually move that one attribute to the next line, and all of the sudden eslint screams about all attributes.

And about indentation level, what do you mean by context? In the first example we know the alignment of the first attribute and simply set expected alignments on the reset of attributes to be equal. And in the second case we set expected indentation on each but the first attribute to be shifted by options.attribute setting value relatively to their parent tag node. Is this what you asked about @chrisvfritz? :)

[chrisvfritz]

What I mean is that in cases like this:

<div id="foo"
class="abc"
style="font-color: red;"
>
</div>

When we fix the indentation to match your proposed style (1 indentation level from the opening tag), how do we know whether to indent 2 spaces, 4 spaces, 1 tab, etc?

[mysticatea]

@michalsnik I think that vertical alignment is enough common style. We have received such request frequently in core as well. Also, the change of default behavior requires mejor version up.

About option form, how about {attribute: {multiplier: number, valign: boolean}}?

[michalsnik]

@chrisvfritz We know it by checking settings of this rule.

@mysticatea the proposition with valign looks nice 👍 Though I still don't like HTML being compared with JavaScript in terms of its preferred style. But if we compare those two - the core indent rule in eslint by default also dosen't expect vertically aligned variables, and the following version is preferred:

const a = 1,
  b = 2,
  c = 3;

ArrayExpression and ObjectExpression also have 1 indent as default ☝️

I think it should look exactly the same here. And since we're still in beta I don't think it would be a big problem to introduce this change now. In my opinion better now than later :)

[mysticatea]

In vue/html-indent, in the const declaration case, the rule aligns vertically. :)

[michalsnik]

Ah, I missed it. Then perhaps this should also be updated 🤔

[mysticatea]

Arrays and objects are the same. If the first element is on the same line as the left paren, it aligns rest elements. The rule handles the condition as the sign of vertical alignment at all. Especially in the variable declaration case, this simplifies implementation hugely.

[mysticatea]

About https://github.com/vuejs/eslint-plugin-vue/blob/master/tests/lib/rules/html-indent.js#L1052-L1087

[michalsnik]

Variables declarations indeed look better vertically aligned, but as for objects, arrays and attributes I think we could quite easily update this rule to be less strict by default. Also cases with attributes, objects or arrays are, or should be most common to use, everything more should rather be avoided in interpolated expressions and bindings in my opinion anyway.

[mysticatea]

I believe it's not "less strict". Just it fits preference or not. The number of spaces is A or B.

I thought the order of commonly is the following. I didn't think third case is intentional. It's the reason that the rule makes vertical alignment if a:1 is not at beginning of line.

let first = {
    a: 1,
    b: 2,
    c: 3
}
let second = { a: 1,
               b: 2,
               c: 3 }
let third = { a: 1,
    b: 2,
    c: 3
}

Anyway, I can accept the change of the default behavior, however, I can't accept if we do without semver-major. We are following to ESLint semantic versioning policy. It intends that bug fixes never break user's CI if the user depends on eslint/plugins with npm's ~. It intends that changes never break user's CI if the user depends on eslint/plugins with npm's ^.

[michalsnik]

Okay let's meet at the middle ground then :) I'll update this PR as I proposed, but under additional option and I won't change the default behaviour to not introduce unnecessary confusion @mysticatea

About that third case - to be honest I didn't see such style in JS ever. Maybe we don't have to change anything about arrays and object in interpolated expressions and bindings at all 🤔 And that optional setting would only take care of HTML attributes indentation. Simple as that.

[michalsnik]

I as promised, I added alignAttributesVertically setting. I thought about attribute: { multipier: 1, valign: true }, but I'm not a big fan of nested configurations. We would have to support both attribute passed as a number and as an object too, that would unnecessarily complicate this setting, so I kept it simple stupid instead.

[mysticatea]

Would you move options.alignAttributesVertically to the call site? processNodeList is a common logic for many expressions/statements. I'd like to change the value of rootNode by the option.

[michalsnik]

Done, check if that's what you wanted me to do @mysticatea :)

[mysticatea]

LGTM, thank you!