Fix conditionals in JSX

[jimbo]

Description

In this codebase, we avoid multiline statements in JSX because they defeat the main purpose of using JSX: readable trees. In practice, this only comes up when conditionally rendering JSX elements. We've established a pattern of first defining conditional elements as variables (constants), then inserting those variables into the final tree, and we've been pretty good about using this pattern. But we haven't been perfect, and that makes the pattern unclear.

This PR corrects all known cases in which we were not adhering to this pattern.

// ❌
return (
  <div>
    {condition ? (
        <Foo />
      ) : (
        <Bar />
      )
    }
  </div>
)

// ✅
const element = condition ? (
  <Foo />
) : (
  <Bar />
)

return (
  <div>
    {element}
  </div>
)

Related Issue

PWA-415

Acceptance

Verification Steps

1. yarn test

Screenshots / Screen Captures (if appropriate)

Checklist

• I have updated the documentation accordingly, if necessary.
• I have added tests to cover my changes, if necessary.

[PWAStudioBot]

	Messages
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖	Associated JIRA tickets: PWA-415.

Generated by 🚫 dangerJS against c04444b

[sirugh]

Is this preferred over <>? Why?

[jimbo]

Yes, it's a bit more explicit and portable. Generally, you can paste our code into a sandbox and it'll work. This particular transform isn't as popular as you might think.

Also, we've used Fragment almost everywhere else.

[revanth0212]

How about using useMemo here? @sirugh and I had a conversation yesterday about this. He can expand more on the useMemo use case.

[jimbo]

We have to draw the line somewhere on memoization, and in this codebase, I've generally drawn that line at element creation. The only place we should be memoizing element creation (afaik) is when we're doing it in a loop.

[sirugh]

when we're doing it in a loop

Like...a render loop? 😆

[sirugh]

But seriously, imagine a toggleable element that we expect a user to interact with quite often. Shouldn't we memoize the state to avoid re-creating the component unnecessarily?

This is the difference we're talking about, I think. Which of these is "more performant"/"better"?

const MyComponent = ({ renderChildren }) => {
    const FooComponent = useMemo(() => {
        return renderChildren ? (
            <Foo />
        ) : null;
    });

    const BarComponent = renderChildren ? (
        <Bar />
    ) : null

    return (
        <Fragment>
            {FooComponent}
            {BarComponent}
        </Fragment>
    )
}

[revanth0212]

Yeah, I feel the same. useMemo can save on garbage collection if we are smart about the dependency array element. useMemo can quickly go out of hand if we abuse it by putting too many elements in the dependency array or elements that have a wide range of possible values. I don't think there is an eslint rule that can help us in this case but I feel useMemo has more potential that we are not using at this point in our code base.

[jimbo]

This is the difference we're talking about, I think. Which of these is "more performant"/"better"?

I feel useMemo has more potential that we are not using at this point in our code base.

Yes, I understand. The difference here is negligible—it's one createElement() call. The benefit is not zero, but it's insignificant.

const MyComponent = props => {
  // this is also a `createElement` call
  // should we memoize it too?
  return <div>{props.text}</div>
}

If we wanted to realize the full potential of memoization, we could reproduce PureComponent.

// let's create a custom hook
// it'll only re-render when props change
const usePureComponent = (Component, props) => useMemo(
  () => <Component {...props} />,
  [props]
)

// looks pure to me
const MyComponent = props => {
  return <div>{props.text}</div>
}

// neat
export default const pure = props => usePureComponent(MyComponent, props)

This is cool, but it's overkill. We're not gaining much from the memoization, and now we're doing something that's pretty unusual. So this is where we draw the line.

We use it to avoid identities changing across renders, and as an early optimization when mapping props to elements in a loop, but not for literally every createElement call that happens to be amenable to memoization.

[jimbo]

But seriously, imagine a toggleable element that we expect a user to interact with quite often. Shouldn't we memoize the state to avoid re-creating the component unnecessarily?

Situationally, any time you have a particularly complex tree, you can consider memoizing it to improve the render performance. Toggling doesn't factor into it much, though.

[sirugh]

A question @revanth0212 had in the office was why not just memoize all the time since you may not know if a tree is complex or not.

[revanth0212]

Same here. Can we use useMemo? At most we will only be caching 2 versions of priceAdjustments either the component or null.