Skip to content

Commit

Permalink
fix unnecessary newlines being added during printing
Browse files Browse the repository at this point in the history
Summary:
There are cases where prettier inserts spaces in between detached nodes.
Mainly this impacted function arguments as we'd do things like:
```
[].map(foo(a, b) {})

// after replacing with an arrow became:

[].map(
  (
    a,

    b,
  ) => {},
)
```

This is bad for obvious reasons - because it is purely junk lines.
From extensive testing, I found that the issue ONLY occurs if there is a docblock.

My hypothesis is that when prettier lays out things like function parameters it looks to see if there is a comment on the line. If there is then it puts the param on the next line.
Though - I don't have a true root cause for this as I haven't properly dug into prettier's source code and it's a pain to do remote nodejs debugging from a devserver and I cannot be bothered checking out locally.

I tried a few fixes, but from my testing this fix (setting detached nodes to have range `[1, 1]`) works for cases of brand new detached nodes, and cloned detached nodes.

Reviewed By: pieterv

Differential Revision: D37499940

fbshipit-source-id: 6398186696692c047a6e4cb65a3901d57ef8745b
  • Loading branch information
bradzacher authored and facebook-github-bot committed Jun 29, 2022
1 parent dd34c0b commit 49366a9
Show file tree
Hide file tree
Showing 11 changed files with 633 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,14 @@
import type {
AnyTypeAnnotation,
CallExpression,
ClassDeclaration,
ExpressionStatement,
FunctionDeclaration,
FunctionExpression,
ImportDeclaration,
Literal,
MethodDefinition,
NewExpression,
ReturnStatement,
VariableDeclaration,
VariableDeclarator,
Expand All @@ -36,11 +40,15 @@ interface FunctionDeclaration_With_id extends FunctionDeclaration {
export type ESQueryNodeSelectorsWithoutFallback = $ReadOnly<{
+AnyTypeAnnotation?: (node: AnyTypeAnnotation) => void,
+CallExpression?: (node: CallExpression) => void,
+ClassDeclaration?: (node: ClassDeclaration) => void,
+ExpressionStatement?: (node: ExpressionStatement) => void,
+FunctionDeclaration?: (node: FunctionDeclaration) => void,
+'FunctionDeclaration[id]'?: (node: FunctionDeclaration_With_id) => void,
+FunctionExpression?: (node: FunctionExpression) => void,
+ImportDeclaration?: (node: ImportDeclaration) => void,
+Literal?: (node: Literal) => void,
+MethodDefinition?: (node: MethodDefinition) => void,
+NewExpression?: (node: NewExpression) => void,
+ReturnStatement?: (node: ReturnStatement) => void,
+VariableDeclaration?: (node: VariableDeclaration) => void,
+VariableDeclarator?: (node: VariableDeclarator) => void,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
/**
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @flow strict-local
* @format
*/

import {t, transform} from './test-utils';

const ARRAY_ITERATOR_METHODS = new Set([
'every',
'filter',
'forEach',
'map',
'some',
// reduce / reduceRight don't accept a context argument
]);

function codemod(code: string) {
return transform(code, context => {
return {
CallExpression(node) {
if (
node.callee.type !== 'MemberExpression' ||
node.callee.computed === true ||
node.callee.property.type !== 'Identifier' ||
!ARRAY_ITERATOR_METHODS.has(node.callee.property.name) ||
node.arguments.length !== 2
) {
return;
}
const callback = node.arguments[0];
const thisContext = node.arguments[1];

switch (callback.type) {
case 'ArrowFunctionExpression':
// arr.forEach(() => {}, this);
// ^^^^ remove
context.replaceNode(
node,
context.shallowCloneNodeWithOverrides(node, {
arguments: [context.shallowCloneNode(node.arguments[0])],
}),
);
break;

case 'FunctionExpression':
if (callback.generator) {
// this isn't possible to directly convert to an arrow function
// the second arg binding is really the only solution
// but really nobody should ever be using an array iterator with a
// generator function - that's just weird.
return;
}
if (thisContext.type !== 'ThisExpression') {
// nobody should be doing anything dumb like this to bind to a weird
// this context, but it is valid code that can't be done with an
// arrow function
return;
}
// arr.forEach(function foo() {}, this);
// ^^^^ remove
// ^^^^^^^^^^^^^^^^^ convert to arrow
context.replaceNode(
node,
context.shallowCloneNodeWithOverrides(node, {
arguments: [
t.ArrowFunctionExpression({
async: callback.async,
body: context.shallowCloneNode(callback.body),
params: context.shallowCloneArray(callback.params),
predicate: context.shallowCloneNode(callback.predicate),
returnType: context.shallowCloneNode(callback.returnType),
typeParameters: context.shallowCloneNode(
callback.typeParameters,
),
}),
],
}),
);
}
},
};
});
}

describe('remove unnecessary array method bind', () => {
it('should work', () => {
const result = codemod(`\
/**
*/
export default class Component {
method() {
return LINKS.map(function (d, idx) {
return 1;
}, this);
}
}
`);

expect(result).toBe(`\
/**
*/
export default class Component {
method() {
return LINKS.map((d, idx) => {
return 1;
});
}
}
`);
});
});
Loading

0 comments on commit 49366a9

Please sign in to comment.