airbnb / javascript

JavaScript Style Guide
MIT License
144.59k stars 26.44k forks source link

Best practice regarding conditional JSX #520

Closed AugustinLF closed 4 years ago

AugustinLF commented 8 years ago

There seems to be several schools regarding the way to write conditional JSX. React's documentation suggests ternary expressions or functions, there are packages using using conditional tags, such as jsx-control-statement.

I settled with ternary when there's not too much logic or when the components are not too big (I then fallback to functions), but ternary expressions can easily get hard to read, and the syntax/spacing really differs, a lot. You can find things without any () around elements, or with weird indentation, to things like that, which readable, feel a bit verbose:

 this.props.condition === 'blah' ? (
    <span>IfBlock</span>
  ) : (
    <span>ElseBlock</span>
  )

Do you folks have an opinion on that?

ljharb commented 8 years ago

Our current usage wavers between conditionally defining a variable above the jsx block (without any multiline ternaries whatsoever), or one of the following:

<div>
  {condition ? (<span />) : null}
  {condition ? (
    <div>
    </div>
  ) : null}
</div>
  {condition && (<span />)}
  {condition && (
    <div>
    </div>
  )}
</div>

The latter example looks cleanest, but it suffers from a footgun: if condition is zero, it will be toString'ed and rendered.

What would be ideal is if JSX had an operator like a ?? b which desugared to a == null ? a : b, but lacking that, and considering the footgun, I'm not sure what we should be recommending for JSX.

ilan-schemoul commented 7 years ago

What about do ?

ljharb commented 7 years ago

@NitroBAY since that's only a stage 1 proposal, and this guide only allows using stage 3 proposals and above, do expressions are not yet relevant - however, since it's an expression, it'd likely be treated the same as any other parenthesized expression.

merlinstardust commented 7 years ago

To avoid the footgun (excellent term and I'll be using it from now on, thank you), wrap the condition in Boolean (or do the !!, but I find Boolean(condition) more readable)

ljharb commented 7 years ago

I believe our eslint config requires over Boolean(), fwiw (if not, it should be). Boolean isn't as safe because I can do global.Boolean = function () { return false; } for example.

NicholasTancredi commented 7 years ago
!loading ? null : (
    <ActivityIndicator 
        size="large"
        color="red"
   />
)

Edit from ljharb recommendation:

!!loading && <ActivityIndicator 
    size="large"
    color="red"
/>
ljharb commented 7 years ago

@w0251251 that's fine! altho often people prefer to have the positive condition go first, leading to cond ? <blah /> : null, which is more awkward, which is why people end up at ‼cond && <blah />.

NicholasTancredi commented 7 years ago

Thanks ljharb, I'll be using this style from now on its much better.

return (
    <View>
        { !loading && <Text>Done</Text> }
        <Text>Something</Text>
        { 
            !!loading && <ActivityIndicator 
                 style={styles.activityIndicator}
                 size="large"
                 color="red"
            />
        }
    </View>
)
benweizhu commented 7 years ago

I always feel it a little bit difficult to read when logic code mix with pure HTML. I would prefer to extract them into a function and use {{this.renderXXX()}}. Unless it's very simple piece of code like {condition ? (<span />) : null} or {condition && (<span />)}

What do you guys think?

ljharb commented 7 years ago

@benweizhu renderFoo methods are not a good idea; these should instead be new components.

In other words, if the logic is too complex to be appropriate inline, then it belongs in a separate component.

benweizhu commented 7 years ago

yeah, agree, just like what react official site said about conditional rendering. My theory is we need to extract the code that exposes too many details and wrap it with something make sense(like another component).

njgraf512 commented 6 years ago

@ljharb, about your comment:

renderFoo methods are not a good idea; these should instead be new components. In other words, if the logic is too complex to be appropriate inline, then it belongs in a separate component.

I agree with this and have seen how refactoring large components with multiple alternate render methods into smaller components has made code easier to reason about, and has also uncovered bugs that were previously unknown.

Other resources that speak to the (in)correctness of renderFoo methods that I've found include:

The reason for my comment here is that I'm working with a collaborator that feels that it should be ok to use multiple render methods within a component. Some questions for you:

Sorry for all the questions, and don't feel compelled to respond as I know you are busy, but I appreciate any insight you can provide!

ljharb commented 6 years ago

@njgraf512 that's not in any way a justification of their use; it's saying that if you do use them, their ordering is specified. The official position of this guide is DO NOT use renderFoo methods. There are still legacy places inside Airbnb where we use them, but whenever the opportunity presents itself, we strive to refactor to SFCs.

njgraf512 commented 6 years ago

@ljharb, thanks, appreciate the quick reply!

BrodaNoel commented 6 years ago

What about using the Conditional Component for JSX / React?

ljharb commented 6 years ago

@BrodaNoel that's just reinventing JavaScript conditionals in XML, which doesn't seem like it's a cleaner approach.

andrew-aladev commented 6 years ago

Please do not force these "best practices", because people takes it too seriously. I've received project with inline hell. I can see 4 cases and 2 nested ternary operators per line. I've received render methods with 40-50 lines of code without spacing. Your best practice makes porridge from code, it is not possible to work with it. People said that found this "best practice" here.

Please edit your comments and add something like "Do not overdose with inline code". Thank you.

njgraf512 commented 6 years ago

@andrew-aladev, where does it say in this thread to use nested conditional logic? If anything the counsel that's been given is to use smaller composable components.

andrew-aladev commented 6 years ago

Hello, @njgraf512. It is so easy for people to reproduce it.

!!loading && <ActivityIndicator

In real world:

!!a && !!((a.w.q || z.a) ? s : r) && (q.e || t.e) && !z && <Route path={v} render={(p) => {
  return ((a.e || !r.q.e) ? <Fit ...
  ...
...
}} /> }) /> )} /> // sparta!!

I know that nobody wont's take responsibility about their "best practice", it is ok. But please sign it specially that your best practice is working only when you have 1 conditional argument and 1 simple component.

Please remember that great community authority means great amount of coders dancing around your advices. Please make your "best practices" more verbose. Thank you.

merlinstardust commented 6 years ago

Long / nested ternaries can be limited. I believe there is a rule already that does this

fredydeltoro commented 5 years ago

Hey i made a component for conditional render is very simple i share with you

import React from 'react';

const Display = (props) => {
  return props.if ? <React.Fragment>{props.children}</React.Fragment> : null;
};

export default Display;

but i have a cuestion why is still evaluating the expresion that does not exist because is null and return an error like this

<Display if={catalog.catalog_pages.length}>
                    <h2>
                      {catalog.catalog_pages[this.state.page].page_name} // this is the expresion that are evaluating an return the error
                    </h2>
      </Display>
otech47 commented 5 years ago

Well I tried googling "eslint indent multiline ternary react" and found nothing but madness. The indent rule for ESLint could use a couple more options but I want to throw out my preferred syntax out there to see if I'm the only crazy person who renders components this way:

<Foo/>
<Bar/>
{condition == true
    ?
        <FooBar
            prop={prop}
        />
    :
        <BarFoo
            prop={prop}
        />
}
<Baz/>
aderiushev commented 5 years ago

@otech47 the same for me, still suffering from the eslint formatting and found no wrokarounds yet

apennell commented 4 years ago

Hopping into this old issue after spending time going through this repo's commit history to defend myself in a PR/check if I'm crazy :sweat_smile: I've spent so much time over the years trying to read between the lines of the JS and the JSX/react styleguides on this subject that I would really like to have a section added that explicitly lays out best practices on this! Seeing as this issue is from 2015 and continues to get comments 4 years later, I think it deserves a section in the docs.

I swear I had seen in this styleguide that the style of rendering jsx with multi-line ternaries is to be written as the initial comment in this issue lays it out:

{condition ? (
  <block />
) : (
  <otherBlock />
)}

So for years I've been writing my ternaries like that. A year ago when I started at a new company, I started seeing two new things. One was ternaries like this:

condition
  ? ifTrue
  : ifFalse;

Aghast, I returned to my bible, which is of course this styleguide doc, and discovered the no-nested-ternary section of the JavaScript styleguide and determined that the question mark goes on the second line if it's JS but that I was still correct and it should be on the first line if it's rendering JSX.

The second thing I saw for the first time here is the use of ternaries for conditional rendering when there's no second option:

{condition && (
  <block />
)}

I again scoured these styleguides trying to figure out what was going and if it was oKaY tO uSe. All I could find was a section in the JS guide explicitly saying NOT to use selection operations in place of control statements. So I took that to mean my coworkers were clever but wrong and I continued to use ternaries with : null in my react code.

Fast forward to today, on a newer team and facing the question "shouldn't the question mark be on the next line?" on a pr and wanting to support my refutation with definitive sources, I returned again to the style guides. This time I couldn't find a single ternary in the whole JSX guide. What I found instead was a new example that included the use of && for conditional rendering. This means that, currently, the JS styleguide says one thing is bad and the JSX/React styleguide uses that same thing as a good example and neither mention the other. Now I have no idea what to think or how to conditionally render JSX 😭 Please clear this up for a poor girl who cares way too much about cOrReCt code style! 🙏

ljharb commented 4 years ago

@apennell both of the first two are fine; the && is dangerous because condition might be 0, so that's bad news bears without !!condition (but only for jsx).

apennell commented 4 years ago

I think that would be worth mentioning, since right now the jsx/react guide includes this as an example:

    // good
    {showButton && <Button />}
sandeepreddy19 commented 4 years ago

{shouldRender && <RenderComponent/>

Vs

`

`

On the conditional renderer component return the children if condition is true

Any advantages of doing 1 vs 2 ?

ljharb commented 4 years ago

The second one seems like a lot of unnecessary indirection and complexity over using simple JS syntax.

sandeepreddy19 commented 4 years ago

The second one seems like a lot of unnecessary indirection and complexity over using simple JS syntax.

@ljharb I heard from a colleague there is little performance benefit in using it as in 2 as the number of times render is called is less than 1?

ljharb commented 4 years ago

@sandeepreddy19 render is always called twice in react's strict mode, and in general can be called many times in the lifetime of an app - but like in all things, performance is one of the least important things - clarity is the most important, and adding a component just to abstract around basic JS syntax is not what i'd consider clear.

luxalpa commented 2 years ago

Use typescript-eslint/strict-boolean-expressions in order to make use of condition && <Component/> statements; for other things, just use explicit comparisons please (array.length === 0 && ...). There's always Boolean or !! but personally I think it's annoying to read. It's a comparison it should act like one. Other languages like Rust, Go, C# and Java don't even allow these implicit booleans at all.

For more complex comparisons it's usually nicer to throw it into a const or a function, like isValidField(f) && <Component/> instead of field?.type == "input" && field.extras.length > 0 && allowEdit && <Component/>

For smaller ternaries, I find /* eslint-disable-next-line no-nested-ternary */ okay, like in this example:

      <Container>
        {/* eslint-disable-next-line no-nested-ternary */}
        {errorMessage !== undefined ? (
          <ErrorMessage error={errorMessage} />
        ) : claim === undefined ? (
          <Spinner size="sm" />
        ) : (
          <ClaimPopupContents claim={claim} onClose={onClose} />
        )}
      </Container>