Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Best practice regarding conditional JSX #520

Open
AugustinLF opened this issue Sep 21, 2015 · 30 comments
Open

Best practice regarding conditional JSX #520

AugustinLF opened this issue Sep 21, 2015 · 30 comments
Labels

Comments

@AugustinLF
Copy link

@AugustinLF AugustinLF commented Sep 21, 2015

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
Copy link
Collaborator

@ljharb ljharb commented Dec 23, 2015

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.

@ljharb ljharb added the question label Dec 23, 2015
@NitroBAY
Copy link

@NitroBAY NitroBAY commented Apr 9, 2017

What about do ?

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Apr 10, 2017

@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.

@merlinpatt
Copy link

@merlinpatt merlinpatt commented Apr 18, 2017

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
Copy link
Collaborator

@ljharb ljharb commented Apr 18, 2017

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
Copy link

@NicholasTancredi NicholasTancredi commented Apr 28, 2017

!loading ? null : (
    <ActivityIndicator 
        size="large"
        color="red"
   />
)

Edit from ljharb recommendation:

!!loading && <ActivityIndicator 
    size="large"
    color="red"
/>
@ljharb
Copy link
Collaborator

@ljharb ljharb commented Apr 28, 2017

@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
Copy link

@NicholasTancredi NicholasTancredi commented Apr 29, 2017

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
Copy link

@benweizhu benweizhu commented Jun 5, 2017

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
Copy link
Collaborator

@ljharb ljharb commented Jun 5, 2017

@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
Copy link

@benweizhu benweizhu commented Jun 7, 2017

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
Copy link

@njgraf512 njgraf512 commented Jan 3, 2018

@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:

  • My collaborator referenced the ordering section of Airbnb's style guide, which lists "optional render methods" as a rationale for their use. Do you have any comment on that, or are you able to speak to the use of these methods within Airbnb?
  • Are you able to provide any more information/anecdotes on the reasoning behind your comments quoted above?
  • Do you know of any other good available resources/documentation that speak to this issue?

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
Copy link
Collaborator

@ljharb ljharb commented Jan 3, 2018

@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
Copy link

@njgraf512 njgraf512 commented Jan 3, 2018

@ljharb, thanks, appreciate the quick reply!

@BrodaNoel
Copy link

@BrodaNoel BrodaNoel commented Feb 17, 2018

What about using the Conditional Component for JSX / React?

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Feb 18, 2018

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

@andrew-aladev

This comment was marked as off-topic.

@njgraf512

This comment was marked as off-topic.

@andrew-aladev

This comment was marked as off-topic.

@merlinpatt

This comment was marked as off-topic.

@fredydeltoro
Copy link

@fredydeltoro fredydeltoro commented Sep 20, 2018

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
Copy link

@otech47 otech47 commented Nov 2, 2018

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/>
@hutsi
Copy link

@hutsi hutsi commented Nov 2, 2018

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

@apennell
Copy link

@apennell apennell commented Oct 10, 2019

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 😅 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
Copy link
Collaborator

@ljharb ljharb commented Oct 11, 2019

@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
Copy link

@apennell apennell commented Oct 11, 2019

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

    // good
    {showButton && <Button />}
@sandeepreddy19
Copy link

@sandeepreddy19 sandeepreddy19 commented Apr 14, 2020

{shouldRender && <RenderComponent/>

Vs

<ConditionalRender condition ={shouldRender}> <RenderComponent/> </ConditionalRenderer>

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

Any advantages of doing 1 vs 2 ?

@ljharb
Copy link
Collaborator

@ljharb ljharb commented Apr 15, 2020

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

@sandeepreddy19
Copy link

@sandeepreddy19 sandeepreddy19 commented Apr 15, 2020

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
Copy link
Collaborator

@ljharb ljharb commented Apr 15, 2020

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.