The implementation was checking if the comment was inside of the expression range, which seems like a good idea. Unfortunately, the expression range is not what's inside of `${}` but the actual AST node, which incidentally doesn't include comments. So the logic was off and returned `undefined` which threw afterwards.
Another solution is to find the first quasi where start is > comment start. This means that the comment appeared between the quasi before and this one... therefore in the expression before!
The flow parser has issues with unicode where it makes node location invalid, there are likely other places where node locations are off. So instead of throwing with a weird error, let's attach it to the first one if it doesn't work.
Fixes#1293
As I was debugging #1248, I found out that the code to fix was actually making things worse. The other two branches are for decorators and deleting some random value of a function. I ran all the tests and the flow object is actually now preserving empty lines and didn't change anything else.
I'd rather remove all those and if something comes up then fix it properly upstream than having those crutches that we don't know why they exist anymore.
In #1257, I discovered that if there's a `""` doc at the end, it's not going to trim the previous one correctly. It also happens to fix a few existing things.
This one is pretty crazy. In #927, I changed
```js
concat(["(", join(concat([", "]), printed), ")"]),
```
into
```js
concat(["(", join(concat([", "]), printedLastArgExpanded), ")"]),
```
which makes the example in #1203 look ugly. The crazy thing is that `JSON.stringify(printed) === JSON.stringify(printedLastArgExpanded)`. So they are deep equal but not the same object. In a non-mutative world, this should cause any problem, but we actually mutate those to propagate breaks.
In the break propagation, we only looked at the first one in case of a conditional group. But, because the two were the same object then it also applied to the second one and happened to be the correct behavior! When I changed that piece of code to be two distinct objects, it no longer worked by accident and caused a bunch of weird issues where breaks didn't propagate correctly.
The solution for this case is to propagate the breaks on all the conditions. I'm pretty sure that this is the expected behavior, we want to deeply recurse in all of them, we don't propagate it up the boundary anyway.
The other use case for `traverseInDoc()` is `findInDoc()`, right now it searches for the first conditional group but it seems very arbitrary. I changed it to not search on any and none of the tests are failing, so I think it's safe(tm). If it triggers weird behavior, then it'll at least be expected and not randomly explode at us if we move groups around.
I tried to go through all the conditions for `findInDoc()` but it triggers a few failures (the output look bad). I'm not really sure why. https://gist.github.com/vjeux/5fb7566cc3d65974817d512d1ef6abe1Fix#1203
In #847, I used a heuristic to find if the element was going to be expanded. But, it wasn't 100% accurate because we couldn't know in which conditionalGroup we would land. We added a way for the parent to tell that function if we should be in `expandLastArg`. By replacing the condition by this variable, it now fixes the issues!
This is so good that adding the right abstraction fixes problems across the board :)
Fixes#997
It turns out that we can't reliably detect with the ast if a comment is before or after the ). In order to fix this same problem with `if` I added the `getNextNonSpaceNonCommentCharacter` function. We can use the same here to fix the problem.
Fixes#933
* Hug template literals inside of JSXExpressionContainer
We already hug a bunch of things inside of `{}`. It seems that it's a good idea to do it for template literals as well. I don't think I've seen anyone actually indent them.
Fixes#1090
* Inline jsx elements with single template literal expression
If there is a single expression and a single template literal, then a lot of people in the jsx-style world inline it. I've also myself used this for markdown and printed it that way. So we probably should print it that way.
Note that I'm checking for children.length === 1, this means that if there's any whitespace, this is not going to be true and will not enter this case. So it WON'T reformat
```js
<style>
{`
color: red;
`}
</style>
```
into
```js
<style>{`
color: red;
`}</style>
```
which is great. You have to opt-in to the second style in order to get it.
Fixes#1090
I'm unclear whether anyone was ever confused by this but the eslint page is kind of compelling
```js
// The intent is not clear
var x = a => 1 ? 2 : 3;
// Did the author mean this
var x = function (a) { return 1 ? 2 : 3 };
// Or this
var x = a <= 1 ? 2 : 3;
```
Adding a parenthesis makes it valid with `{"allowParens": true}` rule. Note that if this option is not enabled, the code would not pass lint in the first place.
I've tried a lot of places where to fix this and this is the only one that gives correct results. This is likely not exhaustive but works for that use case. It's been reported twice in issues and I've seen it happen a few other times so we probably want to get it fixed.
Fixes#922Fixes#797
This one sucks.
The range of the `test` of `if (a /* comment */) {}` is only `a` and doesn't include the comment nor parenthesis. This means that we have no way to know if the comment is placed before or after the `)` unless you look at the actual source and strip all the whitespace/comments characters to see if it's a `)`...
This happened on the babel source code twice and many times in the fb codebase. I think we need to fix it unfortunately :(
Fixes#867
We need to call the `skipToLineEnd` which skips `,` after an inline comment. So we have to put it inside of the while loop as you can have many comments, a comma and then many more comments.
Fixes#964
.#1136 accidentally removed all the empty lines between statements inside of switch cases. I just brough back the logic and made sure to only use it for everything but the last line.
* Add exceptions for html escape usage in href and src attributes.
* Add new test cases.
* Remove useless spaces.
* Do not escape if the parser does not do so.
* Implement a different heuristic for keeping along with the parser.
* Update tests.
* Forgot passing options as param.
* Experimental alternative implementation.
* Remove other function.
* Push test after merge.
* Update getJsXRawValue in order to return the unprocessed raw value.
Latest Babylon version includes a fix that allow us to directly
inject the unprocessed raw value available in the `extra.rawValue`
property of the node. A last transformation is applied by replacing
double quotes to `"` entities.
* Drop unused htmlEscapeInsideDoubleQuote function.
* Move getJSXRawValue function logic to the its only call, drop it.
A simple check is performed to determine if the parser is babylon or
flow via `n.value.extra`. Thus, the corresponding raw value is
extracted. If we are converting a string from single quotes to
double quotes, we need to make sure that double quotes are converted
to ".
* Remove ambiguous comment.
* Add Babylon parser.
* Update test cases accordingly, revert regression introduced previously.
* Extract custom tests from tests/flow/
Approach:
1. Remove all .js files in tests/flow except .snap.js files.
2. Copy over all .js files from tests/ in the flow repo.
3. Go through the diff looking for deletions.
- It was easy to see which deletions were due to changes in the tests
due to updates in the flow repo.
- For the rest of the deletions, I used `git blame` to verify that
they had been added by us since the flow tests were copied over.
This makes tests/flow/ simply a copy of the tests from the flow repo,
making it easier to sync with the upstream flow tests in the future.
* Add a script for syncing the flow tests
* Sync the flow tests
We break on `class` before `extends` but didn't for `interface`. Now we do in the same way. Also TIL that `interface` was an actual keyword in JavaScript :P
* Verify parsers against same snapshot
- Reworked run_spec, now accepts 3th optional array argument for
additional parsers to verify against
- Merged duplicate run_spec configs
- Removed duplicate snapshot data
* Formatted run_spec.js with prettier
* Fixed node4 incompatibility
If you write your code in a functional way where you have an arrow function for each argument, it looks better for them to be inline. I can't imagine any case where it would be used in a different way if we limit to a single argument.
Fixes#1065
We started using the same logic for union and intersection but then added a special case for `a & {}`. It turns out that they should be handled completely differently in practice.
The heuristic i'm using is if you go from object to non-object or vis-versa, then inline, otherwise go to the next line and indent (like &&).
Fixes#864Fixes#1017
This is very common for stateful react functional components. It also matches last object expansion. I think that we should only do that for a single argument in function definitions though, unlike the last for function calls.
Fixes#1019
* Revert "Remove mutation in `printBinaryishExpressions` (#1067)"
This reverts commit e7312ad7b2.
* Revert "Make it clear what parser was used in each snapshot (#1068)"
This reverts commit 4f7ae4815b.
The root cause is that we're calling `printComments` with an empty string, meaning that the leading/trailing comments are not correctly inserted at the right location.
So, the way to fix it is to call `p => concat(parts)` but because we're mutating the array in place, it doesn't work. We need to mutate it and create a copy. But, the root call is actually checking the shape of the parts array which our code is now breaking...
```js
// Don't include the initial expression in the indentation
// level. The first item is guaranteed to be the first
// left-most expression.
parts.length > 0 ? parts[0] : "",
```
The consequence is that binary expressions are no longer indented if the first expression has a comment (but now it places the comment properly!), which seems like a good trade-off.
I'm not sure if we should merge this one or instead refactor this code such that it doesn't rely on mutation and looking at the shape of the printed tree. `printMemberChain` is a good thing to reference for inspiration.
Fixes#946
Right now it's only doing one test for begin/end, but in the issue #1037, we have two nodes that have the same start but different end. The current implementation incorrectly sorts them and the identifier ends up being before the container and therefore the comment search doesn't recurse into it.
Fixes#1037
ast-types was missing a type, I sent a PR on ast-type ( https://github.com/benjamn/ast-types/pull/211 ) which was merged and released (thanks @benjamn for being super quick!). So I'm upgrading ast-types and add the test in this PR.
Fixes#1007
While looking at an instability on the React codebase ( ba1299acc2 (diff-2550aa3d377452ae29361f5e53c51c10) ), I realized that we don't let them break which makes the code look weird.
So I added the ability for them to break :)
We already had a special case for it for a group of 2 elements but now that we are also going in this codepath for 3 if we merge, then we need to do the same here.
Fixes#930
* Add some typescript tests
* fix: Remove extraneous colon in type parameter constraint
* style: Add missing newline at EOF in TS tests
* feat: Pretty print typescript object type annotations
* feat: Pretty print TSFunctionType
* fix: Type annotations was missing on class properties
* Add a new batch of tests
* Bump typescript-eslint-parser
* Add a new batch of tests and fix a syntax error in previous
* Bump typescript-eslint-parser
* Group first arg for inline functions
* Group first arg. Unless there are comments.
* Group first arg. Allow second arg to be empty object/array.
This implementation has a side effect of disallowing empty objects/arrays in the should group last arg heuristic.
* Add handleImportDeclarationComments function.
* Fix indent.
* Update test cases accordingly.
* Implement better heuristic for adding comments.
* Make the test case more exhaustive.
* Refactor condition in printStatementSequence, add new helper function.
* Add new test cases.
* Move logic in SwitchCase case.
* Revert unrelated changes, remove unecessary variables.
* Use util.getLast helper function.
* Move variables out of the loop.
* Simplify code.
* Fix mapping with mutated path.
* Implement checking for not printing the parens in this case.
* Add test case.
* Remove parent type checking.
* Fix test accordingly.
* Refactor to a better heuristic.
* Add new test cases.
* Fix implementation.
* Fix unnecessary function call.
* Add new test cases 🚀.
* Use isObjectTypePropertyAFunction and create isTypeAnnotationAFunction.
* Add missing space.
* Add new test cases.
Printing the first line of a binary expression next to the `=` leads to weird cases where the first expression is parenthesised and doens't read well with chained conditionals as they don't align well. This makes it behave the same was as `if` tests.
Fixes#863
This was intended for object expressions. I tried to remove it for ObjectTypeAnnotation but it changes a ton of stuff as it's used all over the place in many different contexts. We should clean it up but in a later PR :)
Fixes part of #975
In practice, trying to allow calls to be inlined is causing a lot of code to look very weird and unstable as seen by the four issues this is fixing. It also requires us to add a conditional group and do hackery around it.
Fixes#959Fixes#760Fixes#615Fixes#625
Christopher Chedeau