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 MemberExpression functionality in indent rule is a breaking builds #6795
Comments
Chained properties on objects will typically not be indented as the rule expects in variable declarations. For now, ignore variable declarations (as was done in the previous version of ESLint, where all chained properties were ignored) and possibly revisit later to offer users opportunity to enforce alignment and/or sensible indentation.
Looks like we merged in a breaking change. |
Chained properties on objects will typically not be indented as the rule expects in variable declarations. For now, ignore variable declarations (as was done in the previous version of ESLint, where all chained properties were ignored) and possibly revisit later to offer users opportunity to enforce alignment and/or sensible indentation.
@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 Thanks @Trott for keeping on top of this and being so responsive and willing to help! |
According to our semver policy this is not a breaking change -
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 In any case this does not look as a bug, strictly speaking |
btw, this will happen again, if not with this rule but with next one, you should consider using tilde for ESLint dep in your |
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:
So there are two ways to reduce the unintentional disruptiveness of this:
I definitely think #6796 should go in. And I can go either way on #6798. |
(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.) |
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 |
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. |
@nzakas sounds good, what is your assessment of the issue overall? |
@markelog I think what you said above in #6795 (comment) is good.
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 |
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? |
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 |
Yeah, I think it would be a good move, in JSCS we had similar postulation about I suppose this is a bit off-topic though |
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:
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). |
@nzakas wrote:
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. |
Chained properties on objects will typically not be indented as the rule expects in variable declarations. For now, ignore variable declarations (as was done in the previous version of ESLint, where all chained properties were ignored) and possibly revisit later to offer users opportunity to enforce alignment and/or sensible indentation.
#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. |
Yeah I'l give it a shot |
@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. |
@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. |
@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. :) |
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. |
@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. |
Chained properties on objects will typically not be indented as the rule expects in variable declarations. For now, ignore variable declarations (as was done in the previous version of ESLint, where all chained properties were ignored) and possibly revisit later to offer users opportunity to enforce alignment and/or sensible indentation.
@ljharb yes, exactly. |
@connor4312 that's an interesting idea. Can you open a new issue for that? |
Chained properties on objects will typically not be indented as the rule expects in variable declarations. For now, ignore variable declarations (as was done in the previous version of ESLint, where all chained properties were ignored) and possibly revisit later to offer users opportunity to enforce alignment and/or sensible indentation.
@platinumazure yeah, sorry it is unclear. I think it makes sense to do both. |
* Docs: suggest using `~` to version (refs #6795) * fixes
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:
What did you expect to happen?
I expected the indentation to be counted from
bar
and notvar
. Or at least to be able to turn off indentation checking forMemberExpression
s. 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
The text was updated successfully, but these errors were encountered: