Skip to content
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

fix: Format Intersection same as Union #3988

Closed
wants to merge 1 commit into from
Closed

fix: Format Intersection same as Union #3988

wants to merge 1 commit into from

Conversation

albertorestifo
Copy link

Share the same logic to format Interection and Union types in TypeScript
and Flow.

Closes #3986

Share the same logic to format Interection and Union types in TypeScript
and Flow.

Closes #3986
Copy link
Member

@j-f1 j-f1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! LGTM, but I’d like to hear what other maintainers think about it.

@j-f1 j-f1 requested a review from vjeux February 16, 2018 23:21
Copy link
Contributor

@vjeux vjeux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a general rule, if there's complexity in prettier, it probably means that it's there for a reason and we didn't just do it for fun.

For more context, having | be leading was already controversial, as the most common style from what I could tell was to make it trailing. We went through it because it made it line up nicely.

But for &, I've never actually seen people write it in a leading fashion. One of the big reason why is that you very infrequently have to chain them. & is there to build a bigger type, so you usually have one base type and one literal {}.

Worse case is that you will have multiple variables that you chain with one literal. This one isn't super pretty right now but okay and not the common case.

I'm curious do you have a real example that motivated this? In the issue it's just made up code.

// The contents to write to stdin.
stdin?: ?string,
dontLogInNuclide?: ?boolean
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks worse to me

allowHalfOpen?: boolean,
readableObjectMode?: boolean,
writableObjectMode?: boolean
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only one that is arguable, but in my experience it's rare that you combine two variables with an object

@suchipi
Copy link
Member

suchipi commented Feb 17, 2018

I much prefer the current behavior

@@ -321,13 +329,17 @@ type DataBase = {
name: string
};

type UserData = DataBase & {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strongly prefer the inline version and agree with @vjeux's comments.

@j-f1
Copy link
Member

j-f1 commented Feb 17, 2018

How about inlining (for both operators) an identifier/member expression at the beginning xor at the end? i.e.

type foo = Foo & {
  bar: Baz
}

type bar =
  & Foo
  & Bar
  & {
      baz: Quux
    }

@albertorestifo
Copy link
Author

albertorestifo commented Feb 17, 2018

A real-world example of when this formatting comes in handy, is when combining React HOC in a GraphQL-Apollo app, where I needed to construct a type combining many verbose types:

// When creating an HOC
type Props = Subtract<InputProps, WithURLStateProps> &
  RouteComponentProps<ProjectViewRouteParams>;

// A component that uses many HOC
type WithMergeIntentsMutationProps = WithAddIntentToClassifierProps &
  MergeIntentsProps &
  RouteComponentProps<ProjectViewRouteParams>;

Playground

It might not be a common use case, but the formatting proposed in this PR makes this scenario much easier to read IMO.

I like the solution proposed by @j-f1

@vjeux
Copy link
Contributor

vjeux commented Feb 17, 2018

Thanks for the concrete use case. This is the case where it's indeed debatable as to which is the best way. I've personally never seen anyone write those with a leading & in the past. It also feels confusing to me: if there's a space after &, it looks like a binary operator but there's nothing before the first one.

Not sure how involved it would be to implement but adding a newline after = in this case may not be a bad solution:

// A component that uses many HOC
type WithMergeIntentsMutationProps =
  WithAddIntentToClassifierProps &
  MergeIntentsProps &
  RouteComponentProps<ProjectViewRouteParams>;

In any case, I'm reasonably happy with the current output and don't think that we should drastically change it.

@j-f1 j-f1 added the status:wip Pull requests that are a work in progress and should not be merged label May 13, 2018
@j-f1
Copy link
Member

j-f1 commented May 13, 2018

How do we feel about adding some heuristics to avoid breaking in some cases (for example, in Foo & { ... }) and merging this?

@vjeux
Copy link
Contributor

vjeux commented May 13, 2018

@j-f1 we already shouldn’t break for

type A = Foo & {
  a: int
};

@j-f1
Copy link
Member

j-f1 commented May 13, 2018

We do, however, break for multiple identifiers:

Prettier 1.12.1
Playground link

Input:

type A = Foo & Bar & Baz & {
  a: int
};

Output:

type A = Foo &
  Bar &
  Baz & {
    a: int
  };

@albertorestifo
Copy link
Author

I'll close this as inconclusive

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Feb 19, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Feb 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area:intersection types locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. status:wip Pull requests that are a work in progress and should not be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typescript/Flow: Inconsistent union and intersection type multiline formatting
6 participants