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

Typescript/Flow: Inconsistent union and intersection type multiline formatting #3986

Open
albertorestifo opened this issue Feb 16, 2018 · 17 comments
Labels
area:intersection types area:union types lang:flow Issues affecting Flow-specific constructs (not general JS issues) lang:typescript Issues affecting TypeScript-specific constructs (not general JS issues) status:needs discussion Issues needing discussion and a decision to be made before action can be taken type:enhancement A potential new feature to be added, or an improvement to how we print something

Comments

@albertorestifo
Copy link

albertorestifo commented Feb 16, 2018

Prettier 1.10.2
Playground link

--parser typescript

Input:

type IntesectionType = VeryLongTypeA & VeryLongTypeB & VeryLongTypeC & VeryLongTypeD & VeryLongTypeE

type UnionType = VeryLongTypeA | VeryLongTypeB | VeryLongTypeC | VeryLongTypeD | VeryLongTypeE

Output:

type IntesectionType = VeryLongTypeA &
  VeryLongTypeB &
  VeryLongTypeC &
  VeryLongTypeD &
  VeryLongTypeE;

type UnionType =
  | VeryLongTypeA
  | VeryLongTypeB
  | VeryLongTypeC
  | VeryLongTypeD
  | VeryLongTypeE;

Expected behavior:

type IntesectionType = 
  & VeryLongTypeA
  & VeryLongTypeB
  & VeryLongTypeC
  & VeryLongTypeD
  & VeryLongTypeE;

type UnionType =
  | VeryLongTypeA
  | VeryLongTypeB
  | VeryLongTypeC
  | VeryLongTypeD
  | VeryLongTypeE;

I think the Interections type should use the same format as the one used for the Union type.

@j-f1 j-f1 added type:enhancement A potential new feature to be added, or an improvement to how we print something lang:typescript Issues affecting TypeScript-specific constructs (not general JS issues) lang:flow Issues affecting Flow-specific constructs (not general JS issues) labels Feb 16, 2018
@j-f1
Copy link
Member

j-f1 commented Feb 16, 2018

We should probably use the same logic to print both variants, then just swap in the & or | based on the node type. (Implementation is on lines 2250–2349 of printer-estree.js)

@albertorestifo
Copy link
Author

Seems pretty straight forward to fix. I'll try to make a PR during the weekend

@suchipi
Copy link
Member

suchipi commented Feb 27, 2018

For reference: The current behavior is desired. See #3988 (review)

@ericanderson
Copy link
Contributor

The proposed behavior is growing on me. I see the argument about

type bar = foo & {
  baz: number
}

but that could also be special cased.

@j-f1
Copy link
Member

j-f1 commented Feb 28, 2018

It would be nice to special-case it for |, too.

@aaronabramov
Copy link

the case i deal with fairly often is the intersection of functions. here's a small example:

declare var fn: ((aaaaaaaaaaa: "a") => string) &
  ((bbbbbbbbbbbb: "b") => number) &
  ((ccccccccccccc: "c") => boolean);

(fn("a"): string);
// Expected error, because when passing 'b' it's expected to return a number
(fn("b"): string);

// same type with unions
declare var fn2:
  | ((aaaaaaaaaaa: "a") => string)
  | ((bbbbbbbbbbbb: "b") => number)
  | ((ccccccccccccc: "c") => boolean);

// Won't work the same way, all of them are errors
(fn2("a"): string);
(fn2("b"): string);

try flow

it would be nice to have them formatted similar to unions

@aaronabramov
Copy link

i didn't realize the original issue was for typescript. but i guess it's still relevant for flow as well :)

@lydell lydell changed the title Typescript: Inconsistent union and intersection type multiline formatting Typescript/Flow: Inconsistent union and intersection type multiline formatting Aug 17, 2018
@miyaokamarina
Copy link

This is still a problem, IMO. E.g. the following code constructs a type, which includes properties from type A except those present in Bar or given object type (effectively { ...A, ...Bar, ...{ baz: string } }):

type Foo<A> = Pick<A, Exclude<keyof A, 'baz' | keyof Bar>> & Bar & { baz: string };

The intersection expression has three members: Pick<...>, { ... } and Bar, and I expect to see each one on a separate line (if the whole expression doesn’t fit print width):

// Print width: 60.
type Foo<A> =
  Pick<A, Exclude<keyof A, 'baz' | keyof Bar>> &
  { baz: string } &
  Bar;

But Prettier wraps it in pretty unreadable and non-sematic manner:

// Print width: 60.
type Foo<A> = Pick<
    A,
    Exclude<keyof A, 'baz' | keyof Bar>
> & { baz: string } & Bar;
// ↑ What the hell is this? How can I be able to read it?

FYI: Prettier wraps non-type expressions in desired manner:

// RHS expression is almost identical to the RHS expression from above:
const Foo = Pick(A, Exclude(keyof(A), 'baz' | keyof(Bar))) & { baz: string } & Bar;

becomes

// Print width: 60.
const Foo =
    Pick(A, Exclude(keyof(A), 'baz' | keyof(Bar))) &
    { baz: string } &
    Bar;

Also, I agree with @albertorestifo that formatting with leading & is preferable, but it isn’t critical.

@phaux
Copy link

phaux commented Aug 15, 2019

What I wrote:

type IO =
  & (
    | { type: "in"; opts: InputOpts }
    | { type: "out"; opts: OutputOpts }
  )
  & (
    | { mode: "file"; file: string }
    | { mode: "buffer" | "stdio"; stream: PassThrough }
  )

What came out:

type IO = (
  | { type: "in"; opts: InputOpts }
  | { type: "out"; opts: OutputOpts }) &
  (
    | { mode: "file"; file: string }
    | { mode: "buffer" | "stdio"; stream: PassThrough })

:/

Also I just noticed that with --parser flow the output is much better

@tommarien
Copy link

@thorn0 milestone3? as & and | are also logical operators

@alexander-akait
Copy link
Member

@tommarien we can fix it without milestone 3

@tommarien
Copy link

@evilebottnawi i just saw a ressemblance with the logical javascript operators, that is why i suggested the same milestone ;)

@thorn0
Copy link
Member

thorn0 commented Feb 3, 2021

I've closed #2469 and #4047 in favor of this issue. There are good examples of what's wrong with the current formatting of intersection types in these two issues.

@stylemistake
Copy link

I'd argue this should be extended to all operators for consistency, or at least be made configurable, because current situation is incredibly inconsistent, especially if you decide to format TypeScript one way, and JavaScript the other way.

What I mean is this:

const x = string1
  + string2
  + string3

const y = possiblyFalsyExpr1
  || possiblyFalsyExpr2
  || null

@90dy
Copy link

90dy commented Sep 21, 2022

Hi everyone,

I don't really understand why #3988 has not been accepted, and what is intentional by having the current formatting behavior.
When you start having complexe type (with options, inferences, etc) the current formatting prevent the developer to quickly understand your code and eventually end by being unmaintainable.

The arguments put forwards are:

  1. not seen anyone using leading &
  2. looks like a binary operator

For the 1st one, lot of people would like but can't as we mostly use prettier for our syntax formatting, and who's doesn't use tools for formatting are probably not a good example to follow 😅

For the 2nd one, it is the same issue with the pipe | operator. BTW we use this operators for intersection and union because they behave as it would on binary numbers. If there was a XOR (^) operator for typescript types it would work (for object/record it would means "union + removing same keys").

So, can we debates on this and close this issue ?

Thanks

@bradzacher
Copy link

bradzacher commented Apr 20, 2023

I can't help but think that it would be a lot simpler maintenance-wise to just treat intersections the same as unions.

Currently intersections have their own unique logic so can't easily be standardised to format the same as binary expressions - they're going to need their own, custom logic in order to look good - which will increase the maintenance burden over time as more edge cases are found (and introduced by new syntax). Example of bad formatting just for intersections: #14726, #14773.

If we instead formatted multiline intersections the same as unions - with a leading & and each member on its own line - then there would no longer be any additional branches to maintain.

I wonder if it's time to revisit this change with the looming v3 major release?
A major release would be the perfect time to make this sort of change.


As an aside - it's worth noting that this sort of thing doesn't impact flow that much now-a-days because the vast, vast majority of flow object types are merged via spreads {...T, ...U} and intersecting them often leads to weird behaviour in the type system.
This is in contrast to TS where there is no spread syntax so the only options for merging object types is by intersecting them or by using interface extends.

@saltman424
Copy link

Just set up prettier in a large-scale project, and this was definitely a bizarre issue to run into. I would love to see this revisited. The inconsistency between intersection and union has definitely been a hit in readability for our team. Also, my biggest frustration with it is we have a number of intersections that we regularly add to, and not being able to just copy the last item down and edit it is pretty annoying

Also, as mentioned above, the primary case that seems to be used to justify the inconsistency (shown below) seems easy enough to special case, and probably should be special-cased for both intersections and unions. It certainly doesn't feel like there is enough benefit to warrant the inconsistency, and it seems to be less readable in almost anything besides the below use case

#3986 (comment)

type bar = foo & {
  baz: number
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:intersection types area:union types lang:flow Issues affecting Flow-specific constructs (not general JS issues) lang:typescript Issues affecting TypeScript-specific constructs (not general JS issues) status:needs discussion Issues needing discussion and a decision to be made before action can be taken type:enhancement A potential new feature to be added, or an improvement to how we print something
Projects
None yet
Development

Successfully merging a pull request may close this issue.