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

Don't use "export default () => {}" in docs #23575

Closed
gaearon opened this issue Apr 28, 2020 · 20 comments · Fixed by #23600
Closed

Don't use "export default () => {}" in docs #23575

gaearon opened this issue Apr 28, 2020 · 20 comments · Fixed by #23600
Labels
type: documentation An issue or pull request for improving or updating Gatsby's documentation

Comments

@gaearon
Copy link

gaearon commented Apr 28, 2020

See https://twitter.com/JelteHomminga/status/1255230778428010496.

This is a problem because those functions don't get names, so they're anonymous in React DevTools, stack traces, and upcoming features like Fast Refresh.

@gaearon gaearon added the type: documentation An issue or pull request for improving or updating Gatsby's documentation label Apr 28, 2020
@marcysutton
Copy link
Contributor

Thanks for filling the issue, @gaearon! Here's an example of this in the docs, for reference, mentioned in the twitter thread: https://www.gatsbyjs.org/docs/adding-react-components/#importing-react-components

@gaearon
Copy link
Author

gaearon commented Apr 28, 2020

It would also be good to add https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/no-anonymous-default-export.md to the default lint rule set.

@osiux
Copy link
Contributor

osiux commented Apr 29, 2020

Hi, I would like to work on this, wondering what's the preferred way to update to:

Instead of export default () => {}, should we do:

a) export default function Component() {}

or

b)

const Component = () => {};

export default Component;

I'm thinking a would be easier to change, but personally I tend to use b on my apps.

@paul-vd
Copy link

paul-vd commented Apr 29, 2020

Hi, I would like to work on this, wondering what's the preferred way to update to:

Instead of export default () => {}, should we do:

a) export default function Component() {}

or

b)

const Component = () => {};

export default Component;

I'm thinking a would be easier to change, but personally I tend to use b on my apps.

https://twitter.com/dan_abramov/status/1255230047109222400?s=19

@wardpeet
Copy link
Contributor

Let's go with export default function Component() {}. It's one less line + it will make debugging in general easier than an arrow function.

cc @pvdz

@tomadimitrie
Copy link

I don't think option A would be a great idea. Some people tend to replace old-style functions with arrow functions, and when they see they can't directly export a named arrow function in one line they will remove its name without knowing the consequences. I consider option B to be more viable.

@ackvf
Copy link

ackvf commented Apr 29, 2020

@tomadimitrie This rule will warn about it
#23575 (comment)

@tomadimitrie
Copy link

@ackvf yeah, but why use functions only for default exports? Arrow functions are more widespread these days, so in my opinion option B is more suitable. I may be wrong, though

@pvdz
Copy link
Contributor

pvdz commented Apr 29, 2020

Yeah we're also discussing dropping constant arrows altogether as it has the same drawbacks and doesn't really help. So agreed, please change it to export function foo(){}. We're discussing to do the same thing for the Gatsby codebase, as well.

@gatsbimantico
Copy link

Version A
Will use hoisting to set the props, and create an unnecessary scope.

MyComp.propTypes = {};
MyComp.defaultProps = {};

export default function MyComp() {
    console.log(
        MyComp.name, // MyComp
        MyComp.propTypes, // {}
        this, // function MyComp
    );
};

Version B

const MyComp = () => {
    console.log(
        MyComp.name, // MyComp
        MyComp.propTypes, // {}
        this, // undefined
    );
};

MyComp.propTypes = {};
MyComp.defaultProps = {};

export default MyComp;

I would recommend version B.

@LekoArts
Copy link
Contributor

@gaearon

It would also be good to add benmosher/eslint-plugin-import:docs/rules/no-anonymous-default-export.md@master to the default lint rule set.

We're using eslint-config-react-app so when facebook/create-react-app#8926 is merged we'll bump the dependency :)

@pvdz
Copy link
Contributor

pvdz commented Apr 29, 2020

  • Hoisting is not a big concern
  • Setting properties directly on functions is a bad pattern and prone to deoptimization
  • Between const f = () => {} when you mean function f(){} I think the one saying "function" is easier to read, and a lower barrier of entry for people to read the code.

Arrows have their places, but so do function declarations.

@smmoosavi
Copy link

smmoosavi commented Apr 29, 2020

I would recommend version A.

export default function MyComp() {}

In typescript arrow function with generic type is not very friendly

doc generators like "API extractor" evaluate () => {} as variable not functions

I can remember typescript error about "private type exported" when the arrow function pattern used and emit declaration is on.

@eps1lon

This comment has been minimized.

@pvdz

This comment has been minimized.

@eps1lon

This comment has been minimized.

@ackvf
Copy link

ackvf commented May 20, 2020

Though, it should also be noted that you can't type a function declaration to be of type React.FC or any react component type. So for example, you don't know there are children available in props.

Just some quick type hacking:

interface PropTypes {a, b, c}

const Arrow: React.FC<PropTypes> = props => props.children
function Funy(props: PropsWithChildren<PropTypes>): React.ReactElement | null {return props.children}

export type A<T> = Parameters<React.FC<T>>
export type R<T> = ReturnType<React.FC<T>>

interface P { a, b, c }
function Other(...[props, c]: A<P>): R<P> {
  return props.children
}

Funy.., not so fun. We are inclining towards named const exports instead:

export SomeComponent: React.FC<PropTypes> = ...

@muescha
Copy link
Contributor

muescha commented Jun 9, 2020

could we add a summary in some doc to style guide which notation is preferred

or add the eslint plugin (#23575 (comment))?

@pvdz
Copy link
Contributor

pvdz commented Jun 18, 2020

We are going to follow suit to CRA (facebook/create-react-app#8177) and deprecate the pattern, codemod the rest to function declaration style, and lock it down with a lint. The FC deprecation will keep the existing cases for the sake of simplicity but disallow new cases.

This is a work in progress as the conversion codemod is non-trivial.

@NikolayChankov2445
Copy link

I am using version A in my gatsby site , but on Build i get an error :
WebpackError: Element type is invalid: expected a string (for built-in components) or a class/function (for composite components) but got: undefined.
Can that be the reason , and should use B ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: documentation An issue or pull request for improving or updating Gatsby's documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.