New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Prettier #9101

Merged
merged 9 commits into from Mar 14, 2017

Conversation

@sebmarkbage
Member

sebmarkbage commented Mar 3, 2017

Because manual code formatting in so 2016.

A few caveats...

Lint has an issue with some of this stuff. We need to modify lint to exclude the rules that are incompatible with prettier.

We have been inconsistent with line wrapping. We used to strict follow 80 characters but recently we've been more up to 120ish. In this run I used 100 as a compromise to minimize reformatting but we should probably just pick one. 80 characters seems like the way to go.

Overall it seems pretty good!

The change of concatenated string on multiple lines is probably the biggest change.

Show outdated Hide outdated src/isomorphic/classic/class/ReactClass.js
@@ -32,26 +32,22 @@ function identity(fn) {
/**
* Policies that describe methods in `ReactClassInterface`.
*/
type SpecPolicy = /**

This comment has been minimized.

@vjeux

vjeux Mar 3, 2017

Contributor

Fixed by prettier/prettier#853, it'll be in 0.21

@vjeux

vjeux Mar 3, 2017

Contributor

Fixed by prettier/prettier#853, it'll be in 0.21

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Mar 3, 2017

Member

Sweet!

Member

acdlite commented Mar 3, 2017

Sweet!

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Mar 3, 2017

Member

Repushed with --no-bracket-spacing option which is apparently FB standard.

Member

sebmarkbage commented Mar 3, 2017

Repushed with --no-bracket-spacing option which is apparently FB standard.

'https://fb.me/react-animation-transition-group-timeout for more ' +
'information.'
timeoutPropName +
" wasn't supplied to ReactCSSTransitionGroup: " +

This comment has been minimized.

@sebmarkbage

sebmarkbage Mar 3, 2017

Member

This is interesting. Opts to switch to double quote instead of using escapes.

@sebmarkbage

sebmarkbage Mar 3, 2017

Member

This is interesting. Opts to switch to double quote instead of using escapes.

This comment has been minimized.

@vjeux

vjeux Mar 3, 2017

Contributor

You may be interested in the implementation of the string printing.
https://github.com/prettier/prettier/blob/master/src/printer.js#L2836-L2909

@vjeux

vjeux Mar 3, 2017

Contributor

You may be interested in the implementation of the string printing.
https://github.com/prettier/prettier/blob/master/src/printer.js#L2836-L2909

Show outdated Hide outdated src/renderers/dom/shared/ReactDOMComponentTree.js
@@ -47,7 +44,7 @@ function shouldPrecacheNode(node, nodeID) {
*/
function getRenderedHostOrTextFromComponent(component) {
var rendered;
while ((rendered = component._renderedComponent)) {
while (rendered = component._renderedComponent) {

This comment has been minimized.

@sebmarkbage

sebmarkbage Mar 3, 2017

Member

I can see how the over-parenthesization is providing value here. Indicates that I know what I'm doing.

@sebmarkbage

sebmarkbage Mar 3, 2017

Member

I can see how the over-parenthesization is providing value here. Indicates that I know what I'm doing.

This comment has been minimized.

@vjeux

vjeux Mar 3, 2017

Contributor

That's a good point, I can see how it would be useful! Let me try it out.

@vjeux

vjeux Mar 3, 2017

Contributor

That's a good point, I can see how it would be useful! Let me try it out.

This comment has been minimized.

@vjeux

This comment has been minimized.

@jlongster

jlongster Mar 3, 2017

Contributor

👍 I do this too.

@jlongster

jlongster Mar 3, 2017

Contributor

👍 I do this too.

Show outdated Hide outdated src/renderers/dom/shared/ReactDOMComponentTree.js
@@ -95,7 +92,8 @@ function precacheChildNodes(inst, node) {
}
var children = inst._renderedChildren;
var childNode = node.firstChild;
outer: for (var name in children) {
outer:

This comment has been minimized.

@sebmarkbage

sebmarkbage Mar 3, 2017

Member

This seems to indicate to me that labels are available anywhere on their own but they're not actually valid in front of anything - although almost.

@sebmarkbage

sebmarkbage Mar 3, 2017

Member

This seems to indicate to me that labels are available anywhere on their own but they're not actually valid in front of anything - although almost.

This comment has been minimized.

@vjeux

vjeux Mar 3, 2017

Contributor

This is a leftover from the recast fork. I'll remove the \n, it doesn't make sense.

@vjeux

vjeux Mar 3, 2017

Contributor

This is a leftover from the recast fork. I'll remove the \n, it doesn't make sense.

This comment has been minimized.

@vjeux
Show outdated Hide outdated src/renderers/dom/shared/ReactDOMComponentTree.js
node.nodeValue === ' react-empty: ' + nodeID + ' ');
return (node.nodeType === 1 && node.getAttribute(ATTR_NAME) === '' + nodeID) ||
(node.nodeType === 8 && node.nodeValue === ' react-text: ' + nodeID + ' ') ||
(node.nodeType === 8 && node.nodeValue === ' react-empty: ' + nodeID + ' ');

This comment has been minimized.

@vjeux

vjeux Mar 3, 2017

Contributor

@jlongster I really like the decision we made to add parenthesis around && even though they are not necessary. People tend to put them anyway.

@vjeux

vjeux Mar 3, 2017

Contributor

@jlongster I really like the decision we made to add parenthesis around && even though they are not necessary. People tend to put them anyway.

Show outdated Hide outdated src/renderers/__tests__/ReactEmptyComponent-test.js
firstComponent={Child}
secondComponent={'div'}
/>;
var instance1 = <TogglingComponent firstComponent={'div'} secondComponent={Child} />;

This comment has been minimized.

@vjeux

vjeux Mar 3, 2017

Contributor

Btw, the way prettier works is to cram as much as possible within the limit. So it's usually not a good idea to go too far away from 80 columns as the result tend to look crowded.

@vjeux

vjeux Mar 3, 2017

Contributor

Btw, the way prettier works is to cram as much as possible within the limit. So it's usually not a good idea to go too far away from 80 columns as the result tend to look crowded.

Show outdated Hide outdated src/renderers/__tests__/ReactCompositeComponentState-test.js
@@ -226,13 +211,12 @@ describe('ReactCompositeComponent-state', () => {
['forceUpdate', 'blue'],
// unmountComponent()
// state is available within `componentWillUnmount()`
['componentWillUnmount', 'blue'],
['componentWillUnmount', 'blue']

This comment has been minimized.

@vjeux

vjeux Mar 3, 2017

Contributor

you may want --trailing-comma=all

@vjeux

vjeux Mar 3, 2017

Contributor

you may want --trailing-comma=all

This comment has been minimized.

@sebmarkbage

sebmarkbage Mar 3, 2017

Member

We don't do this anywhere else in the codebase though. I'll need to take a minute to process this before I come to terms with it. :P

@sebmarkbage

sebmarkbage Mar 3, 2017

Member

We don't do this anywhere else in the codebase though. I'll need to take a minute to process this before I come to terms with it. :P

This comment has been minimized.

@sebmarkbage

sebmarkbage Mar 3, 2017

Member

I don't know how I feel about this one. It seems odd to read it when so many functions will never have any more arguments. Especially for all the ones that only accept one argument. If I expected it to grow I'd use a "config" object instead.

This formatting seems odd:

function foo(
  {
    x,
    y,
  },
) {}

foo({
  x,
  y,
});
@sebmarkbage

sebmarkbage Mar 3, 2017

Member

I don't know how I feel about this one. It seems odd to read it when so many functions will never have any more arguments. Especially for all the ones that only accept one argument. If I expected it to grow I'd use a "config" object instead.

This formatting seems odd:

function foo(
  {
    x,
    y,
  },
) {}

foo({
  x,
  y,
});
Show outdated Hide outdated src/renderers/shared/fiber/ReactFiberScheduler.js
'you called setState, replaceState, or forceUpdate on an unmounted ' +
'component. This is a no-op.\n\nPlease check the code for the ' +
'%s component.',
(ctor && (ctor.displayName || ctor.name)) || 'ReactClass'

This comment has been minimized.

@sebmarkbage

sebmarkbage Mar 3, 2017

Member

This helps a lot. Nice.

@sebmarkbage

sebmarkbage Mar 3, 2017

Member

This helps a lot. Nice.

Show outdated Hide outdated src/addons/transitions/__tests__/ReactCSSTransitionGroup-test.js
transitionLeave={false}>
transitionName="yolo"
transitionEnter={false}
transitionLeave={false}

This comment has been minimized.

@vjeux

vjeux Mar 3, 2017

Contributor

--jsx-bracket-same-line (btw, all the options have been added to match the fb styleguide, so you want to put them all :p)

@vjeux

vjeux Mar 3, 2017

Contributor

--jsx-bracket-same-line (btw, all the options have been added to match the fb styleguide, so you want to put them all :p)

Show outdated Hide outdated src/isomorphic/modern/class/__tests__/ReactES6Class-test.js
'receive-props', freeze({value: 'bar'}),
'should-update', freeze({value: 'bar'}), {},
'will-update', freeze({value: 'bar'}), {},
'did-update', freeze({value: 'foo'}), {},

This comment has been minimized.

@vjeux

vjeux Mar 3, 2017

Contributor

fyi, if you have those kind of specially structured data, you can add // @prettier-ignore on the line before and prettier will leave it as is.

@vjeux

vjeux Mar 3, 2017

Contributor

fyi, if you have those kind of specially structured data, you can add // @prettier-ignore on the line before and prettier will leave it as is.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Mar 3, 2017

Member

Ok, trying with 80-character limit instead... It does help avoid too crowded lines.

Member

sebmarkbage commented Mar 3, 2017

Ok, trying with 80-character limit instead... It does help avoid too crowded lines.

Show outdated Hide outdated src/__mocks__/View.js
var createReactNativeComponentClass = require('createReactNativeComponentClass');
var createReactNativeComponentClass = require(
'createReactNativeComponentClass',
);

This comment has been minimized.

@sebmarkbage

sebmarkbage Mar 3, 2017

Member

This is a bit unfortunate since often grep for require('x') and I never would thought to include the trailing comma. I suppose this changes with ES modules anyway.

@sebmarkbage

sebmarkbage Mar 3, 2017

Member

This is a bit unfortunate since often grep for require('x') and I never would thought to include the trailing comma. I suppose this changes with ES modules anyway.

This comment has been minimized.

@sebmarkbage

sebmarkbage Mar 3, 2017

Member

This might break www internally actually. :/ (Not this file since it is RN only but this pattern.)

@sebmarkbage

sebmarkbage Mar 3, 2017

Member

This might break www internally actually. :/ (Not this file since it is RN only but this pattern.)

This comment has been minimized.

@vjeux

vjeux Mar 3, 2017

Contributor

This is fixed in master, I'll do a new release tomorrow with those fixes:
prettier/prettier@705ac7d

@vjeux

vjeux Mar 3, 2017

Contributor

This is fixed in master, I'll do a new release tomorrow with those fixes:
prettier/prettier@705ac7d

This comment has been minimized.

@sebmarkbage

sebmarkbage Mar 3, 2017

Member

The nice thing is that prettier always seem to put from and the module name of an ES import on the same line so at least you can grep from 'x' easily.

@sebmarkbage

sebmarkbage Mar 3, 2017

Member

The nice thing is that prettier always seem to put from and the module name of an ES import on the same line so at least you can grep from 'x' easily.

This comment has been minimized.

@vjeux

vjeux Mar 3, 2017

Contributor

(And, it does break www)

@vjeux

vjeux Mar 3, 2017

Contributor

(And, it does break www)

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Mar 3, 2017

Member

In terms of tooling how should we set this up?

We'll probably want CI to run and ensure that nothing gets reformatted.

Do we want the precommit hooks in the repo or should this be a manual step? A manual step is annoying but not too bad. An auto-installed precommit hook in the repo is fairly aggressive. I'd be worried that it would mess something up with no way to revert since the original is not in the history.

Another solution could be a post-commit amend. That way the original commit is still in git reflog.

Member

sebmarkbage commented Mar 3, 2017

In terms of tooling how should we set this up?

We'll probably want CI to run and ensure that nothing gets reformatted.

Do we want the precommit hooks in the repo or should this be a manual step? A manual step is annoying but not too bad. An auto-installed precommit hook in the repo is fairly aggressive. I'd be worried that it would mess something up with no way to revert since the original is not in the history.

Another solution could be a post-commit amend. That way the original commit is still in git reflog.

Show outdated Hide outdated src/renderers/native/__tests__/ReactNativeAttributePayload-test.js
),
).toEqual({
a: [2],
});

This comment has been minimized.

@sebmarkbage

sebmarkbage Mar 3, 2017

Member

Not quite sure I understand the rules of this one. Lots going on in this reformatting, but it's fine.

All the trailing commas are a bit confusing/noisy though.

@sebmarkbage

sebmarkbage Mar 3, 2017

Member

Not quite sure I understand the rules of this one. Lots going on in this reformatting, but it's fine.

All the trailing commas are a bit confusing/noisy though.

This comment has been minimized.

@vjeux

vjeux Mar 3, 2017

Contributor

Oh, I think I know what happened. You started with 120 columns which formatted it as

expect(diff(...)).toEqual({
  a: [2],
});

and we have a rule that says if an object was expanded, we keep it expanded, so when you moved it back to 80 columns, the diff got expanded and this one stayed expanded as well.

That rule is not ideal but this is the best trade-off we could find as there are valid reasons why objects could be inlined or expanded depending on the context.

@vjeux

vjeux Mar 3, 2017

Contributor

Oh, I think I know what happened. You started with 120 columns which formatted it as

expect(diff(...)).toEqual({
  a: [2],
});

and we have a rule that says if an object was expanded, we keep it expanded, so when you moved it back to 80 columns, the diff got expanded and this one stayed expanded as well.

That rule is not ideal but this is the best trade-off we could find as there are valid reasons why objects could be inlined or expanded depending on the context.

This comment has been minimized.

@sebmarkbage

sebmarkbage Mar 3, 2017

Member

I'll squash and rerun from original source.

@sebmarkbage

sebmarkbage Mar 3, 2017

Member

I'll squash and rerun from original source.

@@ -65,8 +65,7 @@ describe('ReactDebugTool', () => {
ReactDebugTool.onTestEvent();
expectDev(console.error.calls.count()).toBe(1);
expectDev(console.error.calls.argsFor(0)[0]).toContain(
'Exception thrown by hook while handling ' +
'onTestEvent: Error: Hi.'
'Exception thrown by hook while handling ' + 'onTestEvent: Error: Hi.',

This comment has been minimized.

@sebmarkbage

sebmarkbage Mar 3, 2017

Member

Ah. String formatting. :)

@sebmarkbage

sebmarkbage Mar 3, 2017

Member

Ah. String formatting. :)

workInProgress.memoizedState,
newState,
newContext,
)

This comment has been minimized.

@sebmarkbage

sebmarkbage Mar 3, 2017

Member

This feels odd. I wonder if there's a pattern to this.

@sebmarkbage

sebmarkbage Mar 3, 2017

Member

This feels odd. I wonder if there's a pattern to this.

This comment has been minimized.

@jlongster

jlongster Mar 3, 2017

Contributor

It's consistent with breaking a function call; if the args are too long it will always put arguments on their own lines. We could see if there is a way to keep it on the same line if there is only one argument, which would also always apply to if/while/etc statements.

We could at least implement this heuristic: if there is only one element inside parens, and it's a unary operator or a function call (maybe other things), print it on the same line. If we do it more generally we'll see things like:

if(thisCondition &&
  thatCondition &&
  anotherCondition) {
  const x = 5;
}

Which is really bad; it's a lot more clear to move the whole expression to the same line (same reason a lot of people put the end bracket in JSX on its own line).

But there's no reason we couldn't optimize for a few patterns. Is it important enough to do so?

@jlongster

jlongster Mar 3, 2017

Contributor

It's consistent with breaking a function call; if the args are too long it will always put arguments on their own lines. We could see if there is a way to keep it on the same line if there is only one argument, which would also always apply to if/while/etc statements.

We could at least implement this heuristic: if there is only one element inside parens, and it's a unary operator or a function call (maybe other things), print it on the same line. If we do it more generally we'll see things like:

if(thisCondition &&
  thatCondition &&
  anotherCondition) {
  const x = 5;
}

Which is really bad; it's a lot more clear to move the whole expression to the same line (same reason a lot of people put the end bracket in JSX on its own line).

But there's no reason we couldn't optimize for a few patterns. Is it important enough to do so?

This comment has been minimized.

@vjeux

vjeux Mar 3, 2017

Contributor

The pattern I've been using at fb is:

if (thisCondition &&
    thatCondition &&
    anotherCondition) {
  const x = 5;
}
@vjeux

vjeux Mar 3, 2017

Contributor

The pattern I've been using at fb is:

if (thisCondition &&
    thatCondition &&
    anotherCondition) {
  const x = 5;
}

This comment has been minimized.

@marvinhagemeister

marvinhagemeister Mar 3, 2017

I've also seen something like this in the wild:

if (thisCondition
&& thatCondition
&& anotherCondition) {
  const x = 5;
}
@marvinhagemeister

marvinhagemeister Mar 3, 2017

I've also seen something like this in the wild:

if (thisCondition
&& thatCondition
&& anotherCondition) {
  const x = 5;
}
inst._currentElement.type,
props,
)
) {

This comment has been minimized.

@sebmarkbage

sebmarkbage Mar 3, 2017

Member

Same thing as previous comment here.

@sebmarkbage

sebmarkbage Mar 3, 2017

Member

Same thing as previous comment here.

{a: [2], b: {k: [3, 4]}, c: {k: [4, 5]}},
{a: true, b: true, c: true},
),
).toEqual({a: [2], c: {k: [4, 5]}});

This comment has been minimized.

@vjeux

vjeux Mar 3, 2017

Contributor

This is correctly putting the argument of toEqual in a single line.

@vjeux

vjeux Mar 3, 2017

Contributor

This is correctly putting the argument of toEqual in a single line.

{a: {diff: diffA}, b: {diff: diffB}}
)).toEqual({a: [2]});
expect(
diff(

This comment has been minimized.

@vjeux

vjeux Mar 3, 2017

Contributor

This is expected that diff( goes to its own line.

@vjeux

vjeux Mar 3, 2017

Contributor

This is expected that diff( goes to its own line.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Mar 3, 2017

Contributor

In terms of tooling how should we set this up?

It's currently uncharted territory, I'm not really sure what the best solution is. I'm waiting to see what people are trying out.

Contributor

vjeux commented Mar 3, 2017

In terms of tooling how should we set this up?

It's currently uncharted territory, I'm not really sure what the best solution is. I'm waiting to see what people are trying out.

@mxstbr

This comment has been minimized.

Show comment
Hide comment
@mxstbr

mxstbr Mar 3, 2017

Contributor

In terms of tooling how should we set this up?

FWIW, I've been using a pre-commit hook with lint-staged in a bunch of projects and it's been nothing but awesome so far. I can code however I like, and in the end I commit and it's formatted however the project wants it. 👌

While I understand the concern about loosing work, the chance of prettier removing something at all is tiny, and then to remove something that's critical and you can't trivially reproduce based on the context is... highly unlikely.

In my opinion, the cost of having to comment under many PRs with failed CIs because the submitters forgot to run prettier is higher than the potential to loose critical, irreplaceable work. This is subjective, so take it with a grain of salt, but I think it makes sense to run it in a pre commit hook.

Contributor

mxstbr commented Mar 3, 2017

In terms of tooling how should we set this up?

FWIW, I've been using a pre-commit hook with lint-staged in a bunch of projects and it's been nothing but awesome so far. I can code however I like, and in the end I commit and it's formatted however the project wants it. 👌

While I understand the concern about loosing work, the chance of prettier removing something at all is tiny, and then to remove something that's critical and you can't trivially reproduce based on the context is... highly unlikely.

In my opinion, the cost of having to comment under many PRs with failed CIs because the submitters forgot to run prettier is higher than the potential to loose critical, irreplaceable work. This is subjective, so take it with a grain of salt, but I think it makes sense to run it in a pre commit hook.

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Mar 3, 2017

Contributor

the chance of prettier removing something at all is tiny

We treat those as "unbreak-now" priority. We ran extensive fuzzing on the codebase to detect this. The one place where there likely are disappearing content is with comments, but we detect when we don't print all the comments and throw an exception instead of silently dropping it.

If you see this happening or prettier generating invalid JavaScript / JavaScript with different semantics, please let me know, I'm going to fix it asap.

Contributor

vjeux commented Mar 3, 2017

the chance of prettier removing something at all is tiny

We treat those as "unbreak-now" priority. We ran extensive fuzzing on the codebase to detect this. The one place where there likely are disappearing content is with comments, but we detect when we don't print all the comments and throw an exception instead of silently dropping it.

If you see this happening or prettier generating invalid JavaScript / JavaScript with different semantics, please let me know, I'm going to fix it asap.

@mxstbr

This comment has been minimized.

Show comment
Hide comment
@mxstbr

mxstbr Mar 3, 2017

Contributor

Well, in that case I don't see any reason not to run it in a pre-commit hook! (and for running it in a pre-commit hook I'd definitely use lint-staged)

Contributor

mxstbr commented Mar 3, 2017

Well, in that case I don't see any reason not to run it in a pre-commit hook! (and for running it in a pre-commit hook I'd definitely use lint-staged)

sebmarkbage added some commits Mar 14, 2017

Fix lint rules
The typeof one is a bit strange because the disable rule gets moved.
@sophiebits

sophiebits approved these changes Mar 14, 2017 edited

A++ please merge this @jlongster!

cartoon drawing of tugboat

@jlongster jlongster merged commit b1b4a2f into facebook:master Mar 14, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@mxstbr

This comment has been minimized.

Show comment
Hide comment
@mxstbr

mxstbr Mar 14, 2017

Contributor

YES!!! Congrats 🎉 🎉 🎉 🎉

Contributor

mxstbr commented Mar 14, 2017

YES!!! Congrats 🎉 🎉 🎉 🎉

@cpojer

This comment has been minimized.

Show comment
Hide comment
@cpojer

cpojer Mar 15, 2017

Contributor

We set it up as a lint step for Jest, similar to how it would work if it was internally at Facebook.

This will throw if files are not printing correctly and tell people what to do. A large part of scripts/prettier.js could be integrated directly into prettier as a --lint function.

Contributor

cpojer commented Mar 15, 2017

We set it up as a lint step for Jest, similar to how it would work if it was internally at Facebook.

This will throw if files are not printing correctly and tell people what to do. A large part of scripts/prettier.js could be integrated directly into prettier as a --lint function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment