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
* WIP immediate feedback
* typescript parser is drop-in replacement for flow parser
* Add new TypeScript Parser snapshots where drop-in replacement possible
* Snapshot updates after rebasing
* Remove unnecessary stripping of properties on TypeScript parser AST
* Remove annotated issues
* Move TS dependencies to dev for now
* Implement new logic for wrapping binary op in arrowFunctionExpression.
* Add new test cases.
* Reuse new helper function in order to fix#917.
* Add new test case.
* Extend heuristic to dive deeper into mixed types.
* Add new test.
* Enhance logic to cover more cases.
* Add new test cases.
* Disable Flow as it gets BindExpression as an unexpected token.
* Simplify getCombinedDeepest function.
* Add missing case.
* Extract all conditions in switch cases to one top level condition.
* Refactor implementation to make it cleaner and also handle ExpressionStatement.
* Update related test cases.
* Add new test case.
* Make condition less expensive.
* Clean up unecessary conditions, simplify condition involving startsWithOpenCurlyBrace.
* Update and add new test cases for better coverage.
* Remove unecessary condition, refactor canBeFirstInStatement to drop some useless parens.
* Update test cases accordingly 🚀.
* Add new handleOnlyComments function.
* Update tests.
* Update test as the printer forces a trailing newline if there were any contents.
* Implement a different heuristic.
* Update tests.
* Add directives checking in handleOnlyComments function.
* Add directives checking in handleOnlyComments function (amend to retrigger CI).
* Remove duplicate.
* Add new helper function to convert comments as blocks in ExportNamedDeclaration.
* Add new test.
* Switch to a leading position.
* Update test accordingly.
The comments infra has been architected with trailing operators and fails for leading `|`. Instead of having leading comments, we can put trailing comments on the previous one and use the same technique as assignment to deal with the indentation.
Fixes#849
This happens very frequently that naming a test makes the entire line go > 80 columns and requires to indent everything which takes a lot more space. We've had this being reported multiple times and it also affects a lot of tests inside of fb.
Fixes#159
(Note, this is built on-top of #841 but github doesn't handle stacked pull requests well...)
This is a leftover from the recast prototype, it hasn't been touched since then. I have never seen anyone not put the label on the same line.
Fixes#859
This is a neat trick from the React codebase. It helps highlight the fact that this is an assignment and not a comparison which is subtle to realize.
Fixes#861
In #605 I restricted it to binary operations as I didn't know how it would affect boolean operations but it turns out the same technique solves problems that people are reporting. So doesn't make sense to restrict it.
Fixes#824
It turns out that my fix was not correct. We should not add parenthesis for FunctionDeclaration instead of checking if the function expression is named.
Fixes#819
In #596, I fixed a bunch of jsx expression comment edge cases and happened to add a softline there. But it turns out that it's not needed and is actually harmful :)
Fixes#712
Since this is extremely rare, I just took the easy way out and if you are adding a comment inside the "as" node, I just move it outside. We could be more fancy but that works.
Fixes#620
This has come up a couple times on the issue tracker on the react-native-web discussion about Twitter internals. It seems like there's a concensus that people don't break long require calls and they'd rather have it go > 80 columns.
Fixes#303Fixes#752
I'm not really sure what a general rule is for those, but starting with LogicalExpressions should be good. Outside of identifiers, function calls and logical expressions, there aren't a lot of things you'd put there in real code anyway.
Fixes#822
Instead of trying to figure out a complicated way to preserve their correct position, it's easier to just migrate those comments before the class.
Fixes#693Fixes#694
Printing a merged group indented was actually not the right fix. The right fix was to print them in a single line. It used to have this behavior when I was mutating the first group but now that I don't anymore I need to reproduce this condition.
Fixes#823
We need to add parenthesis around function expressions if they are named otherwise the name leak, but if the function is not named then it's just superfluous.
The conditional change is due to a bad use of switch case where it would fall through the next one. We don't want parenthesis there.
* Add new handleFunctionDeclarationComments function.
* Add dangling comments printing for function params.
* Add new test case.
* Update tests.
* Refactor handleFunctionDeclarationComments to only addDanglingComment when no params.
* Remove unecessary helper function, only attach dangling comments when no params.
* Reset flow tests, no more regression.
It seems like precedence for this combination of operators is very unclear to people (myself included) and consistently adding parenthesis there would be good.
Fixes#773
The idea is that if you reach the end of the `}` inside of a template literal, we have to flush the trailing comma, otherwise it would generate invalid code. We also need the same special case for JSX.
I don't like adding yet another type of document but it seems like the most elegant way to solve the problem.
Fixes#623
* Add handleTemplateLiteralComment helper function.
* Fix handleTemplateLiteralComment function.
* Extend handleTemplateLiteralComment to deal with trailing comments 🚀.
* Add test.
* Make handle comments function naming more consistent, fix merge conflicts.
* Update tests.
* Add better comment injection in Template Literal.
* Pass options to attach function.
* Update tests to match new implementation.
* Fix let -> var in findExpressionIndexForComment for NodeJS v4.
* Reorder after merge conflicts.
* Drop old tests for dangling arrays.
* Replace redundant conditional by a boolean 🚀.
* Refactor implementation.
Christopher Chedeau