Update: fix indentation of parenthesized MemberExpressions (fixes #8924) #8928
Conversation
The MemberExpression listener in `indent` contains some logic to ensure that if the object of a MemberExpression is wrapped in parentheses, the property is offset from the opening paren, not the object itself. Due to a bug, this logic also caused the property to be offset from the opening paren if the entire MemberExpression was wrapped in parentheses, raather than just the object. This commit updates the MemberExpression listener to specifically check for parentheses around the object, not the entire MemberExpression.
not-an-aardvark
added
accepted
|
@not-an-aardvark, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vitorbal, @gyandeeps and @valorkin to be potential reviewers. |
lib/rules/indent.js
Outdated
| @@ -1217,8 +1217,10 @@ module.exports = { | |||
| const firstNonObjectToken = sourceCode.getFirstTokenBetween(node.object, node.property, astUtils.isNotClosingParenToken); | |||
| const secondNonObjectToken = sourceCode.getTokenAfter(firstNonObjectToken); | |||
|
|
|||
| const tokenBeforeObject = sourceCode.getTokenBefore(node.object, token => astUtils.isNotOpeningParenToken(token) || parameterParens.has(token)); | |||
| const firstObjectToken = tokenBeforeObject ? sourceCode.getTokenAfter(tokenBeforeObject) : sourceCode.ast.tokens[0]; | |||
| const objectParenCount = sourceCode.getTokensBetween(node.object, node.property).findIndex(astUtils.isNotClosingParenToken); | |||
There was a problem hiding this comment.
Wondering if something like sourceCode.getTokensAfter(node.object, { filter: astUtils.isClosingParenToken }).length might be more readable here? As it is, I could see some developers being confused about how the index returned from findIndex would correspond to a count of closing parens.
There was a problem hiding this comment.
It seems like sourceCode.getTokensAfter returns all tokens after the object that match the filter (i.e. it doesn't stop iterating until it reaches the end of the file, or until it reaches the specified token count). So I don't think that will work in this case.
There was a problem hiding this comment.
@not-an-aardvark Hmm... Maybe sourceCode.getFirstTokensBetween(node.object, node.property, { filter: astUtils.isClosedParenToken }).length then?
There was a problem hiding this comment.
Updated to use filter and length. I kept using getTokensBetween rather than getFirstTokensBetween, because I think there's no difference between them when no count is provided.
|
Updated to add the requested test. The example you gave already passes because the |
|
@not-an-aardvark Thanks, makes sense. What about something surrounded by inline parens and also outer parens? (
(foo.bar)
)
.baz |
|
Added that test too. It also passes because there are two sets of parentheses around the object, so |
|
LGTM |
|
@platinumazure Does the updated implementation look good to you? |
eslint-deprecated
bot
added
the
archived due to age
What is the purpose of this pull request? (put an "X" next to item)
[x] Bug fix (#8924)
What changes did you make? (Give an overview)
The MemberExpression listener in
indentcontains some logic to ensure that if the object of a MemberExpression is wrapped in parentheses, the property is offset from the opening paren, not the object itself. Due to a bug, this logic also caused the property to be offset from the opening paren if the entire MemberExpression was wrapped in parentheses, raather than just the object. This commit updates the MemberExpression listener to specifically check for parentheses around the object, not the entire MemberExpression.Is there anything you'd like reviewers to focus on?
Nothing in particular