-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Description
Background
In #737, an alternative format was proposed for nested ternaries, on the grounds that indenting every level of a nested ternary looked bad, especially for long nested ternaries.
However, there were concerns from core maintainer @vjeux that the proposed format would not be compatible with the way we currently do ternaries, and would present problems with long lines.
The community suggested several alternatives, and the one chosen was the simplest – simply flatten them: #5039. Unfortunately, this did not work well in many cases, and will likely be reverted, which brings us back to the drawing board.
It would be nice to come up with a solution for the cases where indented nested ternaries are objectionable prior to merging the revert, to minimize churn in the community. However, I think we should timebox this search to a week or two.
In #9552, I made one attempt at improving nested/chained ternary formatting, but it wasn't great, so I decided to scour proposed solutions and found two candidates that I think could be made to work well.
Note that we may wish to apply these proposals in only certain circumstance, at least at first; more on that below.
Goals
I'd like to find a solution that:
- Scales well to long ternary chains (unlike indents, as in Add indentation back to nested ternaries #9559).
- Transitions well from "single ternary" to "nested/chained ternary".
- Transitions well from "non-JSX" to "JSX" (since I think we really like our current behavior for that use-case).
- Works well for TypeScript Conditionals.
- Works well for simple nested if-else/case chains, as well as more complex situations.
- Causes minimal churn in existing codebases, especially those not using nested ternaries.
- Is concise.
- Clearly highlights the logical structure of the expression.
I don't think something that fully satisfies all of the above constraints, so tradeoffs may be required.
Option A: case-style
const message =
i % 3 === 0 && i % 5 === 0 ? "fizzbuzz" :
i % 3 === 0 ? "fizz" :
i % 5 === 0 ? "buzz" :
String(i);
const paymentMessage =
state == "success" ? "Payment completed successfully" :
state == "processing" ? "Payment processing" :
state == "invalid_cvc" ? (
"There was an issue with your CVC number, and here is some extra info too."
) :
state == "invalid_expiry" ? "Expiry must be sometime in the past." :
"There was an issue with the payment. Please contact support.";
// we would probably just leave non-chained ternaries alone.
const simple = children && !isEmptyChildren(children)
? children
: null
// if we didn't, they'd look like this:
const simple =
children && !isEmptyChildren(children) ? children :
null
const simpleChained =
children && !isEmptyChildren(children) ? children :
component && props.match ? React.createElement(component, props) :
render && props.match ? render(props) :
null
const complexNested =
children && !isEmptyChildren(children) ? children :
props.match ? (
component ? React.createElement(component, props) :
render ? render(props) :
null
) :
null;
type TypeName<T> =
T extends string ? "string" :
T extends number ? "number" :
T extends boolean ? "boolean" :
T extends undefined ? "undefined" :
T extends Function ? "function" :
"object";
type Unpacked<T> =
T extends (infer U)[] ? U :
T extends (...args: any[]) => infer U ? (
SomeVeryLongNameOfSomeKind<U>
) :
T extends Promise<infer U> ? U :
T;
This is the format that was originally proposed in #737.
case-style pros
- Very concise.
- Looks nice in simple cases (eg; fizzbuzz, simple TS conditional types).
- When a consequent or alternate is long, breaks nicely with parens (basically same as JSX-style).
- Suits most goal criteria.
case-style cons
- When the conditional is complex, can be hard to scan and find the
?
, obscuring the program structure. - Looks totally different from a non-chained ternary.
- Not widely used in the wild; some users would be surprised and bothered by the change. It's quite different.
- Could cause substantial (unuseful) git diffs when a ternary is added or removed to a chain.
- If we decide to do "all-JSX-style" if any branch breaks lines, would also cause git diffs to the entire ternary tree when a single condition becomes large.
Incremental adoption
Since this proposal is a little bolder and risks being disruptive, we should ease into it.
I think we should merge #9559, but carve out one or two special cases where case-style clearly makes sense, namely:
- TS Conditional Types.
- Ternary chains with 3 or more cases (and thus more likely to be intended as a "case expression" rather than just an if-else), as suggested by @j-f1 here.
If these were well-received, we could consider adopting the style more broadly at a later time.
If not, the blast radius will be much more limited (both of these cases are likely fairly rare) and we can revert without causing huge amounts of churn.
Option B: if-else-style
This is a compromise between fully flattened (#5039) and fully indented (#9559) that feels more familiar than the solution above:
const message =
i % 3 === 0 && i % 5 === 0
? "fizzbuzz"
: i % 3 === 0
? "fizz"
: i % 5 === 0
? "buzz"
: String(i);
const paymentMessage =
state == "success"
? "Payment completed successfully"
: state == "processing"
? "Payment processing"
: state == "invalid_cvc"
? "There was an issue with your CVC number"
: state == "invalid_expiry"
? "Expiry must be sometime in the past."
: "There was an issue with the payment. Please contact support.";
const simple =
children && !isEmptyChildren(children)
? children
: null
const simpleChained =
children && !isEmptyChildren(children)
? children
: component && props.match
? React.createElement(component, props)
: render && props.match
? render(props)
: null
const complexNested =
children && !isEmptyChildren(children)
? children
: props.match
? (component
? React.createElement(component, props)
: render
? render(props)
: null)
: null;
type TypeName<T> =
T extends string
? "string"
: T extends number
? "number"
: T extends boolean
? "boolean"
: T extends undefined
? "undefined"
: T extends Function
? "function"
: "object";
type Unpacked<T> =
T extends (infer U)[]
? U
: T extends (...args: any[]) => infer U
? SomeVeryLongNameOfSomeKind<U>
: T extends Promise<infer U>
? U
: T;
I first saw this approach outlined here by @JAForbes.
if-else pros
- Clear logical structure; reads very much like an if-else chain.
- Very familiar/similar to our current way of doing ternaries, single or nested.
- Suits most criteria above.
if-else cons
- Subjectively, does not look good for TS Conditional Types (eager for TS community feedback on this).
- Not concise, especially for simple "case expressions" like
paymentMessage
above. - Complex consequents (eg;
complex nested
, above) are a little difficult to read.
Personally, I think this option is strictly superior to both the fully-flat and fully-nested behaviors. However, I'm concerned that it still leaves TS in the lurch.
My take
EDIT: upon further exploration, I think Option A gets too special-case-y and will result in strange git diffs, and big/strange differences in code style that will frequently surprise and confuse users. Unless we were to shift to using it for ternaries everywhere, I think we should not adopt Option A.
Personally, I actually quite like Option B2, proposed by @roryokane below, with a prior implementation by @zeorin in #4767, and I think both Option B and Option B2 warrant further exploration.