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

React/declarations for function components #2414

Conversation

@stefan-wullems
Copy link
Contributor

stefan-wullems commented Sep 18, 2019

This is just draft of how I would like it to work. Any feedback would be much appreciated.

This pull request was inspired by #690

Tasks:

  • warn when component is not defined in the preferred way
  • Autofix component to preferred component
  • able to configure preference both named and unnamed components and autofix both accordingly
  • support autofix for typescript
    • handle components with type arguments
    • ignore function expressions and arrow functions of which the variable declaration is explicitly typed
  • docs
…pe for function components

Fixes #690.
Copy link
Collaborator

ljharb left a comment

Thanks, this is a great start!

It'd be really nice if this could be autofixable. Things to watch out for in the fixers include:

  • any usage of this directly inside the component, or inside an arrow function (ie, not wrapped by another normal function), should prevent the autofix
  • if the component is an arrow function, and has an inferred name, that should be the name of the function expression. if it doesn't, then it should become an anonymous function expression
  • if the component is a function declaration, it should not be autofixed into an arrow expression
  • if the component is a function expression, the explicit name should match whatever the inferred name of the arrow function will eventually be - or else it should not run an autofix.
index.js Outdated Show resolved Hide resolved
lib/rules/declarations-for-function-components.js Outdated Show resolved Hide resolved
lib/rules/declarations-for-function-components.js Outdated Show resolved Hide resolved
@stefan-wullems
Copy link
Contributor Author

stefan-wullems commented Sep 19, 2019

Does autofix take typescript into account at all? I imagine autofixing a functional component that is defined like so:

const Component: React.FC<Props> = (props) => {
   return null
} 

would lead to unintentional side effects. Should eslint-plugin-react care about this? @ljharb

@ljharb
Copy link
Collaborator

ljharb commented Sep 20, 2019

That’s a fair point; if the types can be fixed, great, if not, best to skip auto fixing entirely when there’s type annotations.

@stefan-wullems stefan-wullems requested a review from ljharb Sep 27, 2019
@stefan-wullems stefan-wullems marked this pull request as ready for review Sep 28, 2019
@stefan-wullems
Copy link
Contributor Author

stefan-wullems commented Sep 30, 2019

I will start writing the documentation once I have some certainty that it now works the way you are okay with @ljharb

Copy link
Collaborator

ljharb left a comment

Thanks, this looks great. Let's get that documentation written.

lib/rules/function-component-definition.js Outdated Show resolved Hide resolved
@ljharb ljharb added the new rule label Oct 1, 2019
@stefan-wullems stefan-wullems requested a review from ljharb Oct 7, 2019
@stefan-wullems stefan-wullems requested a review from ljharb Oct 9, 2019
@ljharb
Copy link
Collaborator

ljharb commented Oct 12, 2019

@Stefanwullems tests are failing.

@ljharb ljharb force-pushed the stefan-wullems:react/declarations-for-function-components branch from 50ed041 to 6f07af0 Oct 12, 2019
docs/rules/function-component-definition.md Outdated Show resolved Hide resolved
docs/rules/function-component-definition.md Outdated Show resolved Hide resolved
docs/rules/function-component-definition.md Outdated Show resolved Hide resolved
docs/rules/function-component-definition.md Outdated Show resolved Hide resolved
lib/rules/function-component-definition.js Outdated Show resolved Hide resolved
@stefan-wullems stefan-wullems requested a review from ljharb Oct 13, 2019
@stefan-wullems
Copy link
Contributor Author

stefan-wullems commented Nov 14, 2019

@ljharb If everything is allright, let's get this merged??

@ljharb
Copy link
Collaborator

ljharb commented Nov 29, 2019

@Stefanwullems this needs to be rebased on latest master, ideally squashed down to one commit, and all conflict markers removed (look for <<<<<, >>>>>, and =====).

@stefan-wullems
Copy link
Contributor Author

stefan-wullems commented Nov 30, 2019

@ljharb I have no idea how to squash so it would be perfect if you or someone else can take is off my hands before I explode something.

@ljharb
Copy link
Collaborator

ljharb commented Nov 30, 2019

I’m talking about git rebase; i can certainly try to do it for you but you have more context on the change than i do.

@stefan-wullems
Copy link
Contributor Author

stefan-wullems commented Dec 13, 2019

I’m talking about git rebase; i can certainly try to do it for you but you have more context on the change than i do.

@ljharb I think I rebased it but I have no idea if I did it right.

@stefan-wullems stefan-wullems force-pushed the stefan-wullems:react/declarations-for-function-components branch 3 times, most recently from ce1d96a to d7423f1 Jan 10, 2020
@stefan-wullems
Copy link
Contributor Author

stefan-wullems commented Jan 10, 2020

@ljharb I think it's ready

@stefan-wullems stefan-wullems force-pushed the stefan-wullems:react/declarations-for-function-components branch from 6151282 to cf1cf2e Jan 10, 2020
@ljharb ljharb force-pushed the stefan-wullems:react/declarations-for-function-components branch from cf1cf2e to b8151af Jan 11, 2020
@ljharb
ljharb approved these changes Jan 11, 2020
@ljharb ljharb force-pushed the stefan-wullems:react/declarations-for-function-components branch from b8151af to 99cea84 Jan 11, 2020
@ljharb ljharb merged commit 99cea84 into yannickcr:master Jan 11, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 97.598%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.