Fix: object-curly-newline for flow code #9458
Conversation
The `object-curly-newline` rule made the assumption that the last token is always `}`. However, when an object is typed with flow this is not the case. This causes false positives, as described in babel/babel-eslint#513. By adding a filter to the `getLastToken` function we skip over the flow types, and get the actual closing brace. For symmetry the same filter is added for the open brace, though I'm not aware of any issues it would fix.
|
|
|
LGTM |
There was a problem hiding this comment.
Hi @TiddoLangerak, thanks for the pull request.
Could you please add a test? We have some examples of test fixtures that represent different parsers-- not sure if we have any Flow ones but we have plenty of Typescript examples. Could you please add a test to this rule which uses a fixture you create that contains flow types in the AST, showing that the rule will correctly skip to the closing brace token? Thanks!
aladdin-add
added
bug
- Added tests for `object-curly-newline` in code annotated with flow types - Fixed incorrect behaviour of object-curly-newline for literal object types
|
LGTM |
|
@platinumazure I added tests similar to those found for |
|
I know you guys are busy so I don't want to pressure you, but does someone perhaps have time to take another look at this? |
|
cc @eslint/eslint-team 😄 |
lib/rules/object-curly-newline.js
Outdated
|
|
||
| // With flow, an object can have the form of `{ a : 3 } : { a : number }`. In this form the | ||
| // closing brace isn't simply the last brace. In order to find the closing brace we actually | ||
| // need to traverse through the node and count the braces, to find the matching pair. |
There was a problem hiding this comment.
Is there a way to do this that doesn't involve linearly traversing the node and counting braces? I'm imagining something like this:
if (node.typeAnnotation) {
return sourceCode.getTokenBefore(node.typeAnnotation);
} else {
return sourceCode.getLastToken(node);
}Where node is the same node passed to check, instead of passing the openBrace to this function.
There was a problem hiding this comment.
I vaguely remember looking for that, but couldn't find it. Nevertheless, I'll look again and see if I can make this linear.
There was a problem hiding this comment.
Welp, that exact snippet seems to have done the trick 😄 Thanks for helping me out!
TiddoLangerak
changed the title
| const openBrace = sourceCode.getFirstToken(node, token => token.value === "{"); | ||
| let closeBrace; | ||
|
|
||
| if (node.typeAnnotation) { |
There was a problem hiding this comment.
I wonder if we should move this into astUtils. We have the same code in a bunch of rules already.
ilyavolodin
added
accepted
eslint-deprecated
bot
added
the
archived due to age
What is the purpose of this pull request? (put an "X" next to item)
[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
The
object-curly-newlinerule made the assumption that the last tokenis always
}. However, when an object is typed with flow this is notthe case. This causes false positives, as described in babel/babel-eslint#513.
What changes did you make? (Give an overview)
By adding a filter to the
getLastTokenfunction we skip over the flowtypes, and get the actual closing brace. For symmetry the same filter is
added for the open brace, though I'm not aware of any issues it would
fix.
Is there anything you'd like reviewers to focus on?