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

Require function declaration for named components #33

Merged
merged 2 commits into from
Mar 19, 2022

Conversation

Richienb
Copy link
Contributor

Fixes #32

@sindresorhus
Copy link
Member

Hmm, this will disallow using arrow functions for components at all though. I mostly use function declaration for exported components. It's nice to be able to define using arrow function for non-exported components. Thoughts?

Co-authored-by: Sindre Sorhus <[email protected]>
@Richienb Richienb requested a review from sindresorhus March 19, 2022 09:46
@Richienb
Copy link
Contributor Author

Recently, I've been shifting away from overusing const foo = () => {} because it just defines a function which is what function was built for anyways: function foo() {}. It's the same reason why I defined these functions as actual functions.

Arrow functions are for functions that can fit entirely on one line and that is rarely an entire React component.

@Richienb
Copy link
Contributor Author

Richienb commented Mar 19, 2022

Given this, adding brackets around components that are returned may become more of an annoyance since we would no longer need to make it explicitly clear where our components are since it would be implied by the return keyword. But that's for another issue.

@sindresorhus
Copy link
Member

I agree with your reasoning. Let's try it out.

@sindresorhus sindresorhus changed the title Require function expressions for named components Require function declaration for named components Mar 19, 2022
@sindresorhus sindresorhus merged commit 62e64fd into xojs:main Mar 19, 2022
@Primajin
Copy link

unsolicited opinion here 😉
I am personally going with arrow functions by default for all the things, and only resort to function when I have a clear need for things like this or super

@jonahsnider
Copy link

One thing that I've noticed while updating code to comply with this rule is you can no longer effectively use the TypeScript types provided by @types/react. This bothered me enough to revert the rule change in my own XO config.

Take this snippet for example:

import type {FC} from 'react';

interface Props {
	href?: string;
}

const Button: FC<Props> = props => (
	<a href={props.href} className='btn'>
		<p>{props.children}</p>
	</a>
);

export default Button;

How should this code be updated?

You can turn const Button into export default function Button, but since it's not a variable you can't use FC<T>.
You need to explicitly write the return type and update the Props interface to include the children property which FC<T> adds.

@Richienb
Copy link
Contributor Author

Yes, that's a legitimate use case. We'd need to detect whether TypeScript types are used on the variable which may be difficult. Perhaps it is easier to disable the rule or make it much more lenient?

@Primajin
Copy link

Did you do a performance comparison? Could arrow functions be optimized way more by the JIT than regular functions?

@jonahsnider
Copy link

Arrow functions perform very slightly better since they don't need to allocate a this context. In practice this doesn't matter since almost every React app is getting passed through a bundler pipeline that rewrites code to be ES5 compatible.

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

Successfully merging this pull request may close these issues.

Function expressions for named components
4 participants