Skip to content

Instantly share code, notes, and snippets.

Embed
What would you like to do?
Why check JSXExpressionContainer ancestor instead of JSXElement consequent for whether to use JSXMode.
const WhyShouldTheseBeDifferent = () => {
return (
<div>
If you start with this code:
{showTheThing || pleaseShowTheThing ? (
<Foo attribute="such and such stuff here" />
) : showTheOtherThing ? (
<Bar />
) : null}
And you refactor to this code, it uses the same formatting:
{showTheThing || pleaseShowTheThing ? (
renderFoo({ attribute: "such and such stuff here" })
) : showTheOtherThing ? (
<Bar />
) : null}
But then if you were to refactor further, everything changes.
{showTheThing || pleaseShowTheThing ?
renderFoo({ attribute: "such and such stuff here" })
: showTheOtherThing ?
renderBar()
: null}
Why? I don't think it makes sense.
I think you still want this code:
{showTheThing || pleaseShowTheThing ? (
renderFoo({ attribute: "such and such stuff here" })
) : showTheOtherThing ? (
renderBar()
) : null}
But, honestly, that paren-free code didn't look so bad. Maybe we should use it everywhere?
I explore this question in the next file, below.
</div>
);
};
const WhatIfWeRemovedJSXMode = () => {
return (
<div>
Hmm… this does make me think, though…
We could do away with JSX Mode entirely, and always use the new ternary formatting;
here are some examples:
{showTheThing || pleaseShowTheThing ?
<Foo attribute="such and such stuff here" />
: null}
{showTheThing || pleaseShowTheThing ?
<Foo attribute="such and such stuff here" />
: (
<Bar
attribute="such and such stuff here"
another="more stuff here so the line breaks"
/>
)}
{showTheThing || pleaseShowTheThing ?
<Foo attribute="such and such stuff here" />
: <Bar short />}
Hmm, that wasn't so nice…
We could choose to always break a JSX terminal alternate, eg:
{showTheThing || pleaseShowTheThing ?
<Foo attribute="such and such stuff here" />
: (
<Bar short />
)}
I'm not sure whether we'd want to do that based on the "ancestor is jsx expession container" heuristic,
or the "alternate is jsx element" heuristic:
{showTheThing || pleaseShowTheThing ?
<Foo attribute="such and such stuff here" />
: renderBar()}
vs
{showTheThing || pleaseShowTheThing ?
<Foo attribute="such and such stuff here" />
: (
renderBar()
)}
To play with a real-world example from react-router:
1. JSX Mode:
<div>
{children && !isEmptyChildren(children) ? (
children
) : props.match ? (
component ? (
React.createElement(component, props)
) : render ? (
render(props)
) : null
) : null}
</div>
2. Normal
<div>
{children && !isEmptyChildren(children) ? children
: props.match ?
component ? React.createElement(component, props)
: render ? render(props)
: null
: null}
</div>
3. Break consequents when direct child of JSXExpressionContainer
<div>
{children && !isEmptyChildren(children) ?
children
: props.match ?
component ? React.createElement(component, props)
: render ? render(props)
: null
: null}
</div>
4. Break consequents when indirect child of JSXExpressionContainer
<div>
{children && !isEmptyChildren(children) ?
children
: props.match ?
component ?
React.createElement(component, props)
: render ?
render(props)
: null
: null}
</div>
Honestly most of these seem fine. I think I like 3 best, but 2 works with less special-casing.
What if we take this smaller case and make it more jsx:
{component ? <div>{React.createElement(component, props)}</div>
: render ? <div>{render(props)}</div>
: <div>Nothing is here</div>}
I think that'd look way nicer like this:
{component ?
<div>{React.createElement(component, props)}</div>
: render ?
<div>{render(props)}</div>
: (
<div>Nothing is here</div>
)}
which I think is nicer than this:
{component ? (
<div>{React.createElement(component, props)}</div>
) : render ? (
<div>{render(props)}</div>
) : (
<div>Nothing is here</div>
)}
What about in a loop or something?
{items ?
items.map((item) => {
return (
item.display ? <Item item={item} attr="breaks ternary but not consequent" />
: <Blank />
)
})
: null}
I think this is okay, it doesn't need to be forcibly broken.
Same here:
{showTheStuff && (
foo ? showThing()
: showOtherThing()
)}
And when you mix and match JSXElements with other stuff…
Do you break separately?
{showTheJSXElement ?
<div>the stuff</div>
: renderOtherStuff()}
and
{!showTheJSXElement ? renderOtherStuff()
: (
<div>the stuff</div>
)}
Or together?
{showTheJSXElement ?
<div>the stuff</div>
: (
renderOtherStuff()
)}
and
{!showTheJSXElement ?
renderOtherStuff()
: (
<div>the stuff</div>
)}
Or do you just always break the consequent, only break the alternate if it contains a JSXElement?
{showTheJSXElement ?
<div>the stuff</div>
: renderOtherStuff()}
and
{!showTheJSXElement ?
renderOtherStuff()
: (
<div>the stuff</div>
)}
and
{showRenderThing() ?
renderOtherStuff()
: somethingElse}
Or should the alternate not be special-cased for a JSXElement?
{!showTheJSXElement ?
renderOtherStuff()
: <div>the stuff</div>}
and
{!showTheJSXElement ?
<Foo />
: <Bar />}
…Nah, a JSXElement alternate should always break. Anything else should be as-needed.
Okay, and what about attributes?
<Foo
oneLine={isRedColor ? red : null}
twoLines={
isRedColor ? allTheRedColors
: someGreenColors
}
withJSXConsequent={
isRedColor ? <RedColorThing />
: someGreenColors
}
withJSXAlternate={
isRedColor ? someRedColor
: <GreenColorThing />
}
/>
… pretty clearly should have no special-casing, default behavior is great with or without JSXElements.
We should probably special-case a null-consequent to stay on one line:
{hideTheStuff || dontShowTheStuff ? null : (
<MyComponent />
)}
</div>
)
}
// Outside of a JSXExpressionContainer:
const x =
component ?
<div>{React.createElement(component, props)}</div>
: render ?
<div>{render(props)}</div>
: (
<div>Nothing is here</div>
)
// or
const x =
component ? <div>{React.createElement(component, props)}</div>
: render ? <div>{render(props)}</div>
: <div>Nothing is here</div>
// honestly I think the latter is what people would want. More compact, still easy to see where the JSX is.
// So I think, ultimately, the *only* JSX special-case algorithm should be:
// When a ternary's parent is a JSXExpressionContainer which is not in a JSXAttribute,
// break the consequent,
// and when the alternate is a JSXElement, break the alternate.
@rattrayalex

This comment has been minimized.

Copy link
Owner Author

@rattrayalex rattrayalex commented May 16, 2021

This elaborates on topics raised in https://gist.github.com/rattrayalex/dacbf5838571a47f22d0ae1f8b960268

EDIT: the conclusions from this stream-of-consciousness reasoning are now present in the above gist

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