Eslint: indent rule with array of objects

[kentor]

Thought I would file a separate issue instead of hijacking https://github.com/eslint/eslint/issues/3456. Using eslint 1.2.1. I expected the following to pass:

eslintrc:

---
rules:
  indent: [2, 2, {VariableDeclarator: 2}]

index.js:

var a = 1,
    b = 2;

var c = [
  {
    d: 1
  }
];

index.js
  5:3  error  Expected indentation of 4 space characters but found 2  indent
  6:5  error  Expected indentation of 6 space characters but found 4  indent
  7:4  error  Expected indentation of 4 space characters but found 2  indent

✖ 3 problems (3 errors, 0 warnings)

This will pass, but just looks weird:

var c = [
    {
      d: 1
    }
];

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

[BYK]

This looks like the correct behavior since you've set VariableDeclarator to 2. Why do you expect it to not error?

[ilyavolodin]

Agree with @BYK Looks like correct behavior to me. I understand that everyone has their own style, but to me your style looks very strange.

[kentor]

Ok consider this for my expectation not to error:

// The reason for [2, 2, {VariableDeclarator: 2}]
var foo = 1,
    bar = 2;

// Passes
module.exports = [
  {
    foo: 1
  }
];

// Does not pass
var bar = [
  {
    foo: 1
  }
];
module.exports = bar;

In the last block, by changing from exporting the array directly to exporting a variable assigned with an array, I now have to indent the object literal which seems wrong to me.

Maybe another way to think of it is this, which could be an option to the indent rule: if I initialize a variable that spans multiple lines, ignore the value of VariableDeclarator, as in the case of module.exports = ....

[BYK]

In the last block, by changing from exporting the array directly to exporting a variable assigned with an array, I now have to indent the object literal which seems wrong to me.

That is the whole purpose of having VariableDeclarator: 2 in your options. Remove that, and you don't have to do it.

[kentor]

Then the first block will error, which is the reason for setting VariableDeclarator: 2.

[leahciMic]

@BYK @kentor this was not an issue in v1.0.0. I believe it started happening around 1.1.0.

I think @BYK is right in that eslint is doing what is asked.

Perhaps this could be another configuration option left to the user to decide on what is or isn't right in their code base. That code style may look strange, but eslint is configurable and you can certainly enforce 'strange' code styles with it.

[BYK]

@BYK @kentor this was not an issue in v1.0.0. I believe it started happening around 1.1.0.

We had a bunch of fixes and improvements over the indent rule after 1.0.0 so it is possible that it got introduced afterwards. That said it doesn't make this a bug :)

Perhaps this could be another configuration option left to the user to decide on what is or isn't right in their code base. That code style may look strange, but eslint is configurable and you can certainly enforce 'strange' code styles with it.

Although I agree with you about supporting as many styles, even if they look strange, I also think we shouldn't do it at the cost of clarity to many other users. I think adding a new option to ignore/reset indentation for nested arrays and objects for variable declarations would make the rule way too complicated (since it is already complex).

My personal suggestion is forking the rule, making the change and using it as a plugin. That said I'd wait someone else on @eslint/eslint-team to weigh in for a definitive answer.

[ilyavolodin]

I agree. I'm very hesitant to add any new options to indent rule. It's already quite unwieldy, adding any additional complexity to it should have very clear justification.

[scottohara]

Just my 2c, but I tend to agree with @kentor in that:

// These all look OK to me (but fail the rule)

var foo = [
  {a: 10, b: 20},     // <- objects in array indented by 2 spaces
  {a: 11, b: 21},
  {a: 12, b: 22}
];                    // <- no indent, aligns with 'var'


var foo = [
  {                   // <- objects in array indented by 2 spaces
    a: 10,            // <- properties of object indented by 4 spaces
    b: 20
  },
  {
    a: 11,
    b: 21
  },
  {
    a: 12,
    b: 22
  }
];                    // <- no indent, aligns with 'var'


var foo = [
      {a: 10, b: 20}, // <- objects in array indented by 6 spaces
      {a: 11, b: 21},
      {a: 12, b: 22}
    ];                // <- indented by 4 spaces, aligns with 'foo'


var foo = [
      {               // <- objects in array indented by 6 spaces (2 more than 'foo')
        a: 10,        // <- properties of object indented by 8 spaces
        b: 20
      },
      {
        a: 11,
        b: 21
      },
      {
        a: 12,
        b: 22
      }
    ];                // <- indented by 4 spaces, aligns with 'foo'


// These all look 'weird' to me (but pass the rule)

var foo = [
    {a: 10, b: 20},   // <- objects in array indented by 4 spaces, aligns with 'foo'
    {a: 11, b: 21},
    {a: 12, b: 22}
];                    // <- no indent, aligns with 'var'


var foo = [
    {                 // <- objects in array indented by 4 spaces, aligns with 'foo'
      a: 10,          // <- properties of objects indented by 6 spaces
      b: 20
    },
    {
      a: 11,
      b: 21
    },
    {
      a: 12,
      b: 22
    }
];                    // <- no indent, aligns with 'var'

I appreciate this is only personal preference; but I thought I should weigh in with another vote for @kentor's preferred style.

I have no particular suggestion on what config options (if any) might be appropriate, though.

[targos]

I think @kentor example should be legal. It is basically the same as https://github.com/eslint/eslint/issues/3352

[silverwind]

How about making VariableDeclarator only apply to single-line statements?

[targos]

or only when there are two or more declared variables

[silverwind]

This also seems wrong advice to me:

indent: [2, 2]

var foo, bar = {
  a: 'b'
};

     2:3   error  Expected indentation of 4 space characters but found 2  indent
     3:1   error  Expected indentation of 2 space characters but found 0  indent

[ilyavolodin]

@silverwind could you expand on your suggestion about single-line statements? VariableDeclarator was added to allow aligning multi-line declarations on name. I'm not sure I understand how would you limit it to single-line statements.

[chemoish]

Slightly tangental…

indent: [2, 4]

var foo = [
  { // <— 2:3  error  Expected indentation of 4 space characters but found 2  indent
        a: 'b'
  } // <— 4:3  error  Expected indentation of 4 space characters but found 2  indent
];

var bar = [
  {a: 'b'}, // <— Expect to fail (passes)
   {b: 'c'}, // <— Expect to fail (passes)
    {c: 'd'}, // <— Expect to *pass*
     {d: 'e'} // <— Expect to fail (passes)
];

I am thoroughly confused with the variations of indent rules that pass/don't pass (@silverwind's and variation).

• Is the above expected?
• Does this fall into the single-line statement case?

[silverwind]

@ilyavolodin can't recall my train of thought there, but my initial assumption that this issue is related to VariableDeclarator was proven wrong anyways, so it shouldn't matter.

[kentor]

Just leaving a note here, I've switched to enforcing one-var: never to get around this issue.

[nzakas]

@gyandeeps @byk can you comment on this?

[gyandeeps]

Based on the issue, I think its the expected behavior under the VariableDeclarator flag. Sometimes its difficult to have both sides of the world legit because then when do we say something is wrong.

I think this whole discussion is going against the expected behavior of VariableDeclarator flag.

[silverwind]

@gyandeeps: It seems most if not all of the issues here are related to multi-line Object and Array literals. Maybe an exception could be made for these so they don't strictly have to follow the indent level of VariableDeclarator?

[BYK]

:+1: to @gyandeeps

[nzakas]

So I think we are in agreement that this is the expected behavior given the configuration. The question note is if we can add an option to allow this as an exception?

This is a fairly common pattern, so I'm sure the request will come up again.

[esteban-uo]

+1 to adding an exception for this case

[jadengore]

:+1: for this case, just updated from <1.0 and this is a problem in many spots.

[erbridge]

Another case where this seems to behave weirdly:

const foo = function foo() {
  bar(
    'arg',
    {  // Expected indentation of 8 space characters but found 4
      key: 'value',  // Expected indentation of 10 space characters but found 6
    }  // Expected indentation of 8 space characters but found 4
  );
};

With the following rules:

{
  "indent": [
    2,
    2,
    {
      "SwitchCase": 1,
      "VariableDeclarator": {
        "var": 2,
        "let": 2,
        "const": 3
      }
    }
  ]
}

[brianac]

I've also encountered the use case above, where we have objects in our parameters that are erroring when they shouldn't.

For now we're going to turn off the variable declarator and put our lets and consts on the same line.

Adding my vote for a exception for this as well. We'd appreciate it

[matanh-tzmedical]

+1

[ExplodingCabbage]

This is pretty obviously a bug. I don't understand why people are suggesting otherwise?

The indentation rules that are applied to arrays are different to those applied to objects here, for no clear reason.

@BYK and @gyandeeps, are you saying that you think it's reasonable that any usage of the VariableDeclarator rule to enforce discrepancies in the indentation of arrays versus objects such as the following (with indent of 2 and VariableDeclarator of 2):

let foo = [
    // Nested object must be indented four spaces to appease eslint
    {
      qux: 'baz'
    }
];

let bar = {
  // Nested object must be indented two spaces to appease eslint
  key: {
    moo: 'cow'
  }
};

Because that's the behaviour being complained about, and it's not clear to me whether the people saying that this is expected behaviour understand that or not. I don't see any reason why eslint should be enforcing different rules for indentation of elements in arrays versus elements in lists; I've never encountered any indentation standard that did this and consider it a pretty unambiguous bug.

[Kos]

An elegant solution would be to add a parameter like VariableDeclarator: "hanging" that would work like (assuming indent with 2 spaces):

let a = 10,
    b = 20,
    c = {
      foo: bar
    };
const d = 30,
      e = 40;
let f = [
  10, 20, 30
];

This is analogous to typical hanging indentation with parentheses:

letfunc(a,
        b,
        {
          foo: bar
        });
constfunc(d,
          e);
letfunc([
  10
]);

If you set indent to 4 spaces, this would become:

let a = 10,
    b = 20,
    c = {
        foo: bar
    };
const d = 30,
      e = 40;
let f = [
    10, 20, 30
];

I'd see this supersede VariableDeclarator: <Number>, I don't think it makes sense to continue looking at the indentation for let and const as 'multiples of normal indentation'.

[ExplodingCabbage]

I'd see this supersede VariableDeclarator: , I don't think it makes sense to continue looking at the indentation for let and const as 'multiples of normal indentation'.

No argument from me - to be honest, it's unclear to me whether the "multiple of normal indentation" approach ever made sense in the first place. Is there actually any use case for it other than allowing declarations to be lined up vertically?

[nzakas]

I like the suggestion, does one of you want to try prototyping so we can take a look?

[platinumazure]

No argument from me - to be honest, it's unclear to me whether the "multiple of normal indentation" approach ever made sense in the first place. Is there actually any use case for it other than allowing declarations to be lined up vertically?

To be completely honest, I never thought it made sense either. It just happened that var/let/const plus space were multiples of 2, and so could be made to work with that common 2-space indentation style. But with this style, it is impossible to have the default indentation be 4 and the const be 6 (for alignment) without getting into floating point stuff, which just seems weird. We really should just be saying what the proper indent should be, not using multiples.

[karan-kang]

Another use case:

I use an array object to define the HTML template in JS, especially when I am using angular template cache. The indentation helps in distinguishing parent and child nodes and improves tempalte readability.

Example:

[
  '<div>',
        '<span>This is a text</span>',
   '</div>'
].join('')

[BYK]

@Karankang007 - I cannot understand how your example is related to the discussion.

[jadengore]

@Karankang007 agree with @BYK, these won't even be joined with line breaks so the representation you have is a moot point.

[karan-kang]

I was just giving an example of indent in array object. Don't mind me.

[BYK]

@Karankang007 - it is okay to give examples. Actually if you feel strongly for the example you provided, you may propose it as an enhancement to the rule, but it should be in its own issue. Our responses were just to mentioned that the example you have provided does not really apply to this specific issue :)

[cowboy]

I just ran into this issue. This is what I'm seeing:

// Eslint complains about this with {VariableDeclarator: 2}
var arr = [
  {
    prop: 1,
  },
  {prop: 2},
  {
    prop: 3,
  },
];

// And doesn't complain when I set it to this.
var arr = [
    {
      prop: 1,
    },
  {prop: 2},
    {
      prop: 3,
    },
];

Seems weird to me. Either way, I removed {VariableDeclarator: 2}, which is almost never applicable in my code, since I have one-var set to {uninitialized: 'always', initialized: 'never'}.

[jadengore]

@cowboy that is my typical issue as well, except I use the VariableDeclarator rule. Our entire codebase has had to change to your second example.

[catkins]

Still facing this too 😞

[UnsungHero97]

I'll chime in as well with my use case. When it comes to chaining promises, I prefer the first code style over the second:

// 1.
return Promise
  .resolve(...)
  .then(() => {...})
  .catch(() => {...});

// 2.
return Promise.resolve(...)
.then(() => {...})
.catch(() => {...});

So the problem I'm facing is related to the first code style:

function importCryptoKey(rawKeyBytes) {
  ...
  // SubtleCryptoAPI is basically a wrapper around window.crypto.subtle
  return SubtleCryptoAPI
    .importKey(
      'raw',
      rawKeyBytes,
      {                // Expected indentation of 4 space characters but found 6
        name: 'PBKDF2' // Expected indentation of 6 space characters but found 8
      },               // Expected indentation of 4 space characters but found 6
      false,
      ['deriveKey']
    )
    .then((cryptoKey) => {...})
    .catch((e) => {...});
};

To make ESLint happy, I need to extract a const above, or shift back the 3 lines, or collapse them into a single line as { name: 'PBKDF2' }.

[nzakas]

Thanks everyone for the feedback. It's not helpful to have more people say they're experiencing the same issue. We are aware, that's why the issue is open. The "help wanted" label means we need someone from the community to take ownership of this issue and drive it forward to create a prototype (https://github.com/eslint/eslint/issues/3493#issuecomment-178060080).

[jadengore]

@nzakas Maybe lock this issue from further comments?

[nzakas]

@jadengore we need to leave it open so someone can express interest in addressing it.

[ExplodingCabbage]

Isn't @UnsungHero97's example actually a different issue, unrelated to the var/let/const indent rules that this issue is about? His example probably needs its own issue, doesn't it?

There are a bunch of unhelpful "me too" comments above his, but his post is something new. If it's unhelpful, it's only by virtue of being in the wrong place.

[UnsungHero97]

all, apologies for the spam. my use case seems relevant to this issue, but I'd be happy to open a new issue if that's not the case.

[kaicataldo]

Closing this as we haven't seen movement on it from either team members or the community in months.

See this comment for context.

[ExplodingCabbage]

I don't understand the closure. Is this a statement that fixes to the bug are now unwelcome? If not, why close the issue?

[ilyavolodin]

@ExplodingCabbage Our policy specifies that issues that haven't been accepted in 21 days should be closed. We had at one point over 300 issues open, and what we learned is that issues that haven't been accepted within the first couple of weeks, tend to stay in that state basically forever, because with so many open issues, nobody has time to go through them all to find something they want to work on. If the issue is truly a problem, it will be opened again at some point in the future, and we will re-evaluate it at that point.

[stschindler]

@ilyavolodin Does "accepted" mean "confirmed"? Because this issue has been confirmed, see discussion.

[ilyavolodin]

@stschindler No, accepted means that the team feels that this issue should be fixed. While the issue might be confirmed, it might be too much of an edge case for us to fix. Or nobody might have time to fix it. We are all volunteers and work on ESLint on our free time. If nobody wants to fix an issue, it will just be sitting there open forever:-(

[stschindler]

@ilyavolodin I see, guess it's a policy thing then. Personally I find it not ideal to close issues that are about confirmed bugs, but it is like it is. I'd love to help out, but I have no time to spend on this, unfortunately. :-( Thanks for the clarification.

[not-an-aardvark]

I think this might be fixed by #7618 anyway.

[stschindler]

@not-an-aardvark Ohhh that's great, I didn't find that one. Awesome, thanks!