Skip to content

Conversation

typeofweb
Copy link

@typeofweb typeofweb commented Apr 3, 2020

Fixes #7940

My intention was to only affect nested conditional types in TypeScript and not to introduce any changes to how ternary expressions are formatted. This allows for a smoother transition, however, it might yield seemingly inconsistent results (see this example).

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/pr-XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

Try the playground for this PR

@typeofweb typeofweb changed the title Issue 7940 Ugly formatting of nested conditional types [TypeScript] Apr 3, 2020
@thorn0
Copy link
Member

thorn0 commented Apr 3, 2020

The old (i.e. current) formatting looks more readable if the constituent types aren't as short and simple as in your examples.

Prettier pr-7948
Playground link

--parser typescript

Input / Output, v2.0.2:

export type ReadonlyDeep<T> = T extends
  | Primitive
  | ((...arguments: any[]) => unknown)
  ? T
  : T extends ReadonlyMap<infer KeyType, infer ValueType>
  ? ReadonlyMapDeep<KeyType, ValueType>
  : T extends ReadonlySet<infer ItemType>
  ? ReadonlySetDeep<ItemType>
  : T extends object
  ? ReadonlyObjectDeep<T>
  : unknown;

Output, this PR:

export type ReadonlyDeep<T> =
  T extends Primitive | ((...arguments: any[]) => unknown) ? T :
  T extends ReadonlyMap<infer KeyType, infer ValueType> ?
  ReadonlyMapDeep<KeyType, ValueType> :
  T extends ReadonlySet<infer ItemType> ? ReadonlySetDeep<ItemType> :
  T extends object ? ReadonlyObjectDeep<T> :
  unknown;

@typeofweb
Copy link
Author

typeofweb commented Apr 3, 2020

I think it still looks more readable including changes from this PR.

Input:

export type ReadonlyDeep<T> = T extends Primitive | ((...arguments: any[]) => unknown) | SuperLongTypeVeryLong
	? T
	: T extends ReadonlyMap<infer KeyType, infer ValueType>
	? ReadonlyMapDeep<KeyType, ValueType>
	: T extends ReadonlySet<infer ItemType>
	? ReadonlySetDeep<ItemType>
	: T extends object
	? ReadonlyObjectDeep<T>
	: unknown;

PR:

export type ReadonlyDeep<T> =
  T extends
    | Primitive
    | ((...arguments: any[]) => unknown)
    | SuperLongTypeVeryLong ? T :
  T extends ReadonlyMap<infer KeyType, infer ValueType> ?
  ReadonlyMapDeep<KeyType, ValueType> :
  T extends ReadonlySet<infer ItemType> ? ReadonlySetDeep<ItemType> :
  T extends object ? ReadonlyObjectDeep<T> :
  unknown;

master:

export type ReadonlyDeep<T> = T extends
  | Primitive
  | ((...arguments: any[]) => unknown)
  | SuperLongTypeVeryLong
  ? T
  : T extends ReadonlyMap<infer KeyType, infer ValueType>
  ? ReadonlyMapDeep<KeyType, ValueType>
  : T extends ReadonlySet<infer ItemType>
  ? ReadonlySetDeep<ItemType>
  : T extends object
  ? ReadonlyObjectDeep<T>
  : unknown;

@thorn0
Copy link
Member

thorn0 commented Apr 3, 2020

I disagree. I'd prefer the new formatting for the example from #7940 and the old formatting for this ReadonlyDeep example (from here).

This looks especially bad:
image

@typeofweb
Copy link
Author

Alright! Would you mind pasting the second example with your suggested formatting?

@thorn0
Copy link
Member

thorn0 commented Apr 4, 2020

The current formatting works great for that example.

@typeofweb
Copy link
Author

Indeed. But what if you have both long and short types inside a nested conditional?

@thorn0
Copy link
Member

thorn0 commented Apr 4, 2020

Ideally, the new formatting should be used only if each condition with its "true" branch fits on one line like in your original example, but as far as I understand Prettier's printer algorithm doesn't support such a trick (changing the formatting of previous lines if the current line exceeds the print width). This means we'll have to go for a less perfect solution: a heuristic that would check the AST and decide whether the new formatting would fit within the print width.

Comment on lines -154 to +171
type Foo = (
ThingamabobberFactory extends AbstractThingamabobberFactory
? GobbledygookProvider
: CompositeGobbledygookProvider
) extends DoubleGobbledygookProvider
? UniqueDalgametreService
: CompositeZamazingoResolver;

type Foo2 = DoubleGobbledygookProvider extends (
ThingamabobberFactory extends AbstractThingamabobberFactory
? GobbledygookProvider
: CompositeGobbledygookProvider
)
? UniqueDalgametreService
: CompositeZamazingoResolver;

type Foo3 = (
ThingamabobberFactory extends AbstractThingamabobberFactory
? GobbledygookProvider
: CompositeGobbledygookProvider
) extends (
DoubleGobbledygookProvider extends MockGobbledygookProvider
? MockThingamabobberFactory
: ThingamabobberFactory
)
? UniqueDalgametreService
: CompositeZamazingoResolver;
type Foo =
(
ThingamabobberFactory extends AbstractThingamabobberFactory ?
GobbledygookProvider :
CompositeGobbledygookProvider
) extends DoubleGobbledygookProvider ? UniqueDalgametreService :
CompositeZamazingoResolver;

type Foo2 =
DoubleGobbledygookProvider extends (
ThingamabobberFactory extends AbstractThingamabobberFactory ?
GobbledygookProvider :
CompositeGobbledygookProvider
) ? UniqueDalgametreService :
CompositeZamazingoResolver;

type Foo3 =
(
ThingamabobberFactory extends AbstractThingamabobberFactory ?
GobbledygookProvider :
CompositeGobbledygookProvider
) extends (
DoubleGobbledygookProvider extends MockGobbledygookProvider ?
MockThingamabobberFactory :
ThingamabobberFactory
) ? UniqueDalgametreService :
CompositeZamazingoResolver;
Copy link
Contributor

Choose a reason for hiding this comment

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

These should remain unchanged.

@rattrayalex
Copy link
Collaborator

FYI #9561 contains proposals to use this style of formatting in TS Conditionals as well as, possibly, some nested ternaries. I wrote it before I saw this PR, but I might just close mine in favor of this.

Do you still plan to work on it? Would you like help handling cases where the clauses are long?

Base automatically changed from master to main January 23, 2021 17:13
@kube
Copy link

kube commented Feb 15, 2021

This is something that hurts me since I started using Prettier on TypeScript libraries.

Usage of conditional types in this domain happens a lot more than in classical Applications, and I also think that the output is ugly.

It does not make any sense that the first condition is always next to =, like in:

export type SchemaToType<S extends Schema> = S extends SchemaLiteral
  ? SchemaLiteralToType<S>
  : S extends SchemaPrimitive
  ? SchemaPrimitiveToType<S>
  : S extends Schema.Record
  ? SchemaRecordToType<S>
  : S extends Schema.Array
  ? SchemaArrayToType<S>
  : S extends Schema.Intersection
  ? SchemaIntersectionToType<S>
  : S extends Schema.Union
  ? SchemaUnionToType<S>
  : S;

Just allowing to have the first condition on the next line (maybe through an config option?), would be a big win IMO.

export type SchemaToType<S extends Schema> =
  S extends SchemaLiteral
  ? SchemaLiteralToType<S>
  : S extends SchemaPrimitive
  ? SchemaPrimitiveToType<S>
  : S extends Schema.Record
  ? SchemaRecordToType<S>
  : S extends Schema.Array
  ? SchemaArrayToType<S>
  : S extends Schema.Intersection
  ? SchemaIntersectionToType<S>
  : S extends Schema.Union
  ? SchemaUnionToType<S>
  : S;

It makes it much easier to read, particularly when there are multiple generics (with defaults etc...).

@kube
Copy link

kube commented Feb 15, 2021

Any news on this @mmiszy @thorn0 ?
Can I help somehow ?

@thorn0
Copy link
Member

thorn0 commented Feb 15, 2021

@kube I've opened a separate issue for that: #10325

@thorn0
Copy link
Member

thorn0 commented Feb 15, 2021

Doing a clean up of stale PRs, closing this. It's linked to the issue so we can revive it if necessary.

@thorn0 thorn0 closed this Feb 15, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ugly formatting of nested conditional types in TypeScript
5 participants