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

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Member

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 marked this pull request as ready for review September 28, 2019 15:31
@stefan-wullems
Copy link
Contributor Author

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

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

lib/rules/function-component-definition.js Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented Oct 12, 2019

@Stefanwullems tests are failing.

@ljharb ljharb force-pushed the react/declarations-for-function-components branch from 50ed041 to 6f07af0 Compare October 12, 2019 22:30
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
Copy link
Contributor Author

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

@ljharb
Copy link
Member

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

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

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

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 react/declarations-for-function-components branch 3 times, most recently from ce1d96a to d7423f1 Compare January 10, 2020 09:51
@stefan-wullems
Copy link
Contributor Author

@ljharb I think it's ready

@stefan-wullems stefan-wullems force-pushed the react/declarations-for-function-components branch from 6151282 to cf1cf2e Compare January 10, 2020 16:14
@ljharb ljharb force-pushed the react/declarations-for-function-components branch from cf1cf2e to b8151af Compare January 11, 2020 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants