New MemberExpression functionality in indent rule is a breaking builds

[Trott]

What version of ESLint are you using?

3.2.0

What parser (default, Babel-ESLint, etc.) are you using?

default

Please show your full configuration:

indent: 2

What did you do? Please include the actual source code causing the issue.

This throws an error:

var foo = bar.baz()
            .bip()
            .boop();

What did you expect to happen?

I expected the indentation to be counted from bar and not var. Or at least to be able to turn off indentation checking for MemberExpressions. So I expected no error.

What actually happened? Please include the actual, raw output from ESLint.

error Expected indentation of 2 space characters but found 12

[kaicataldo]

Looks like we merged in a breaking change. MemberExpression (which was not being linted before) is now defaulting to 1 for its indentation level.

[kaicataldo]

@eslint/eslint-team We should fix this right away. @hzoo and I have already experienced this in our CI build at work.

Relaying info from the Gitter chat:

We actually can't have this on by default - even setting the option to 0 is a breaking change because we weren't checking MemberExpressions at all previously. Seems like the only way to make this not a breaking change is to have the MemberExpression check opt-in and off by default.

Thanks @Trott for keeping on top of this and being so responsive and willing to help!

[markelog]

According to our semver policy this is not a breaking change -

Minor release (might break your lint build)
A bug fix in a rule that results in ESLint reporting more errors.

So these changes are completely justified since they were released in minor.

If we not provide an option for this and make your preference by default, then another user might pop up with the complete opposite request saying that this -

var foo = bar.baz()
    .bip()
    .boop();

Is now showing errors.

Having said that, if this change is as invasive as you said it is, then, in my eyes, we could consider following approach - revert that change, provide a specific option for MemberExpressions without default and release it in another minor while considering setting default for major release.

In any case this does not look as a bug, strictly speaking

[markelog]

I have already experienced this in our CI build at work.

btw, this will happen again, if not with this rule but with next one, you should consider using tilde for ESLint dep in your package.json if you don't want such breakages in future

[Trott]

Not an ESLint member, but since my code caused the issue and I'm offering up two fixes (that are non-exclusive--they both could land), I guess I feel like I should say something:

• I agree that it's debatable if this was a breaking change or not. I think the real issue (as is alluded above) is just how disruptive is it.
• As it stands, it is terribly disruptive (in my opinion).

So there are two ways to reduce the unintentional disruptiveness of this:

• Fix: ignore MemberExpression in VariableDeclarators (fixes #6795) #6796 changes things so that MemberExpression is not linted in VariableDeclarator and AssignmentExpression. This will resolve the "trying to comply with this lint rule makes my code ugly and hard to read" issue for the vast majority of users.
• Fix: Make MemberExpression option opt-in (fixes #6797) #6798 changes the option to be opt-in. This should fix it for everyone, I would think.

I definitely think #6796 should go in. And I can go either way on #6798.

[Trott]

(And, of course, reverting the change entirely is always an option. That would make me sad, but that's not a reason to avoid it.)

[markelog]

Oh, so there is a option for this, wasn't aware of it, then yeah, it seems we are already following my proposal, heh.

I like this style btw -

var foo = bar.baz()
  .bip();

:), matter of style is very subjective, I vote for #6798.

Also, I brought up a revert as a quick fix only, so we could figure this out thoroughly with time on our hands so your changes could have been incorporated later in the following commit(s), but it seems you are faster then revert

[nzakas]

Our policy is not to rush out fixes. We tend to do releases on Friday so we affect as few people as possible and then do patch releases on Monday if necessary. So, let's not rush the fix for this so we can be sure we are solving the problem correctly.

[markelog]

@nzakas sounds good, what is your assessment of the issue overall?

[hzoo]

@markelog I think what you said above in #6795 (comment) is good.

ESLint follows semantic versioning. However, due to the nature of ESLint as a code quality tool, it's not always clear when a minor or major version bump occurs. To help clarify this for everyone, we've defined the following semantic versioning policy for ESLint:

Minor release (might break your lint build)
A bug fix in a rule that results in ESLint reporting more errors.

I don't think anyone is suggesting the default be 0 or 1 just that any default was set. But according to the policy that will happen. I think we just need to signal that if you don't want a "breaking change / breaking lint build" (in terms of other projects) you should be using ~ in package.json for ESLint since it can be surprising.

[nzakas]

First, it's definitely not the worst post-release issue we've had, so we can all relax. :)

Then, I think we have to go back to ask the question, what was the original intent of the change? Was it to just indent chained MemberExpressions only if they weren't in an assignment? Or was the intent to cover all of the instances? @Trott

@hzoo that's a good point. Want to put together a PR for the readme with that suggestion?

[ljharb]

I think one concern with the "indent" rule particularly is that because each of the suboptions accepts an integer, there's no way to disable that category of checking.

I think if that was possible - by passing null, or -1, or "off", or something - then it wouldn't be so disruptive if a minor version added a new category type.

[nzakas]

Oops, forgot to mention, I think #6798 is along the lines of what we probably should have done initially. I'm not sure of #6796 without getting a better understanding of the original intent.

[markelog]

@hzoo that's a good point. Want to put together a PR for the readme with that suggestion?

Yeah, I think it would be a good move, in JSCS we had similar postulation about ^ and ~. Users would need to be accustom to this practise though, maybe we could put some parts about semver policy in our github template.

I suppose this is a bit off-topic though

[kaicataldo]

I honestly didn't know/had forgotten about our policy on minor releases allowing for breaking changes in those situations. Looks like this has been a productive discussion, but I'd like to make clear that I wasn't suggesting an alternative default.

I actually agree with this:

If we not provide an option for this and make your preference by default, then another user might pop up with the complete opposite request

which is why it seemed like having the option be entirely opt-in and off by default made sense to me. That being said, given this discussion, I agree that that doesn't seem like the best way forward anymore.

Sorry for the false alarm - was trying to nip it in the bud if it was a problem (which it doesn't seem like it is).

[Trott]

@nzakas wrote:

Then, I think we have to go back to ask the question, what was the original intent of the change? Was it to just indent chained MemberExpressions only if they weren't in an assignment? Or was the intent to cover all of the instances? @Trott

I did not specifically seek to exclude anything originally but I do think it's fair to say that the intention was to deal with all situations where consistent indentation would be obvious and unsurprising. Judging from what I've seen in projects I work on and some of the things that have cropped up elsewhere, that would seem to exclude assignments.

If I haven't missed anything, it seems like there is consensus that #6798 (make it opt-in only) would be A Good Thing. I suspect there will eventually be consensus that #6796 (ignore declarations and assignments) is an improvement over the current behavior too, but that becomes a whole lot less crucial if #6798 is in place.

[nzakas]

Yeah, I think #6798 is the right fix. I would, however, like to get consensus on whether #6796 makes sense. Basically, I'd rather have all the pain of these changes bundled together instead of doing a slow drip of pain to minimize the negative impact.

[Trott]

#6796 makes things more permissive so I think if we are looking to minimize pain, landing it in addition to #6798 is the way to go. With #6796, variable declarations and assignments will be treated exactly as they were before today's release.

We can always add configuration options later to address variable declarations and assignments. I believe there has been talk of changing the way indentation options are specified in the next major release. If so, that would be an opportunity to revisit.

[hzoo]

Want to put together a PR for the readme with that suggestion?

Yeah I'l give it a shot

[nzakas]

@Trott sounds like a good plan.

On the plus side, the lack of activity on this issue means it's not been a huge deal for the community.

[ljharb]

@nzakas i think the same argument you made in #6795 (comment) means you absolutely can not extrapolate that it's not a huge deal just yet.

[nzakas]

@ljharb I'm not sure why you're arguing this point, but let me try to explain.

When we have a significant problem affecting a lot of people, the issue fills up very quickly with references to other issues and pull requests in other projects. This tends to happen within a couple hours of release (which are always on Friday). So by Saturday, there are usually people screaming about how horrible it is and how we messed up all of their CI, etc.

Releasing on Friday limits the impact, but it's still very obvious when something is a significant problem for the community. We've seen nothing of that sort here.

So, I'd ask that you trust that we know by now how to determine when something is a significant problem. We've been at this for three years, after all. :)

[connor4312]

Maybe adding an alerting system would be beneficial for ESLint. It looks like Greenkeeper immediately caused failures on ~100 repos (including our own) after the last ESLint version was pushed, it would be simple to create a script that scrapes that a few minutes after publishing. If a non-major version causes an abnormal amount of Greenkeeper "breaking" PRs, that is probably a pretty good indicator that something is wrong.

[ljharb]

@nzakas Understood, and sorry if it seemed I was stepping on toes. Is the implication that if you released on, say, a Monday, you'd just have a much larger number of people yelling, but that by releasing on Friday, you still get the response, just at a more manageable level? If so, then that makes sense, sorry.

[platinumazure]

Do we have consensus on accepting both #6798 and #6796, or just #6798? Hard to tell with the side conversations.

[nzakas]

@ljharb yes, exactly.

[nzakas]

@connor4312 that's an interesting idea. Can you open a new issue for that?

[nzakas]

@platinumazure yeah, sorry it is unclear. I think it makes sense to do both.