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

Docs: Javascript style guide - multi-line arrow functions are okay without braces #9203

Closed
samouri opened this issue Nov 8, 2016 · 7 comments

Comments

@samouri
Copy link
Contributor

samouri commented Nov 8, 2016

As sparked in a conversation here, @spen had the idea that we should revise our javascript style guide to allow for multiline arrow functions without braces.

The current style guide explicitly says:

// bad
// it's too magic, it works only with parentheses, otherwise it returns collection of undefine values
[ 1, 2, 3 ].map( x => ( { value: x * x } ) );

But it is becoming an increasingly common pattern to utilize brace-less multiline arrow functions.
What is everyones thoughts ? Is it a good idea to update the docs and allow items like so:

list.map( item =>
  <div>
    <li> item.name </li>
  </div>
);
@brunoscopelliti
Copy link
Contributor

I think we should distinguish the two cases...

In my opinion this is good:

list.map( item =>
  <ul>
    <li>{ item.name }</li>
  </ul>
);

because the single root element provides the same visual help of a couple of parentheses.
It also works without further parentheses cause the babel react-preset reduces it to a single expression. You can note how the arrow function was not transpiled in this babel repl demo.

On the other hand the first example can't work correctly without parentheses. The parentheses are needed to help the engine distinguish the object literal { value: x * x } from a block with a label statement.
In this case, I think we should be biased through the most readable form, even at the cost of a few more key strokes. That being said, I think that @spentay has a good point here:

the first example labelled as 'bad' uses parens for execution but claims that doing so is 'too magic'... Since it seems to be a fairly common pattern for returning components it might be worth revising

That is, now that people are getting more and more familiar with ES2015, to return an object literal by wrapping it into parentheses is becoming an idiomatic form.

@spen
Copy link
Contributor

spen commented Nov 10, 2016

@spentay had the idea that we should revise our javascript style guide to allow for multiline arrow functions without braces.

I'm actually for using braces on multi-line.
As much as I do visually prefer :

list.map( item =>
    <li>
        <span>Name: { item.name }</span>
    </li>
);

I still like to add the parens:

list.map( item => (
    <li>
        <span>Name: { item.name }</span>
    </li>
) );

I don't feel too strongly about this as I do find this opinion hard to defend.
One thing though: I think it keeps consistency between implicit and explicit returns:

list.map( item => {
    const name = capitalize(item.name);
    return (
        <li>
            <span>Name: { name }</span>
        </li>
    );
) );

return needs the ( ... ) wrapper to avoid ASI from doing return;

Also AirBnB recommends using parens for multi-liners.

Testing the following on chrome & FF actually works fine but I still don't completely trust ASI not to error when omitting the parens (and not using babel).

a = b =>
1
console.log( a() ) // 1

Still I feel like there's room for extra clarity here - I'd vote for adding wrapping parens to any multi-line implicitly returning arrow functions (MLIRAFs for short 😂)

I've got a PR coming with a couple of edits to the JS guidelines as I didn't realise there was one in progress (I think it's merged now though).
I'll open a separate one that might better show my thoughts on this :)

@brunoscopelliti
Copy link
Contributor

brunoscopelliti commented Nov 11, 2016

@spen you've a good point on consistency between implicit and explicit returns.
In fact this could also be useful if you've to refactor a single line arrow function to a multi lines arrow function.

@spen
Copy link
Contributor

spen commented Nov 12, 2016

@aduth's PR #9037 (merged) does cover this topic under the Iteration heading.

This seems fine to me. If anything it could be slightly more explicit about what we've discussed above but I'm on the fence! Thoughts?

@aduth
Copy link
Contributor

aduth commented Nov 14, 2016

#9037 also removed the text from the original comment that referred to the arrow function usage as being "bad". To me personally, specific usage of arrow functions falls on the side of developer preference, not warranting an explicit guideline one way or the other.

@samouri
Copy link
Contributor Author

samouri commented Dec 26, 2016

Closing because I feel this issue was resolved in #9037.
If anyone feels different feel free to reopen

@stale
Copy link

stale bot commented Jan 11, 2018

This issue has been marked as stale because it hasn't been updated in a while. It will be closed in a week.
If you would like it to remain open, can you please you comment below and see what you can do to get things moving with this issue?
Thanks! 🙏

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

No branches or pull requests

5 participants