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

[new] add static-property-placement rule #2193

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@dmason30
Copy link

dmason30 commented Mar 10, 2019

This adds a rule discussed in these two issues:

I have based the options on @ljharb suggestions from both issues. This is my first attempt at writing a rule so I expect plenty of feedback.

  • static public field (ClassProperty)
  • static getter {MethodDefinition)
  • property assignment (MemberExpression)
...
"react/static-property-placement": [<enabled>, <string>, {
  childContextTypes: <string>,
  contextTypes: <string>,
  contextType: <string>,
  defaultProps: <string>,
  displayName: <string>,
  propTypes: <string>,
}]
...
  • Also I have never worked with TypeScript/Flow so I am not sure what (if any) additional changes are needed?

  • This rule only applies to ES6 classes.
    -- Ignored: Stateless functional components can only ever have a MemberExpression to declare any of the above properties.
    -- Ignored: The React.createClass/createReactClass can only ever have a Property to declare any of the above properties.

  • This does not have autofix either but I feel that is a "nice to have" option and could come later.

dmason30 added some commits Mar 10, 2019

@dmason30

This comment was marked as resolved.

Copy link
Author

dmason30 commented Mar 11, 2019

This is "Ready for review", sorry I opened as a draft and I am now unable to change it.

@ljharb ljharb added the new rule label Mar 12, 2019

@ljharb ljharb marked this pull request as ready for review Mar 12, 2019

@ljharb
Copy link
Collaborator

ljharb left a comment

I'd also like this form to be incorrect code:

class Foo extends React.Component {
  static get defaultProps() {}
}

So perhaps we need a third option, static getters?

Show resolved Hide resolved docs/rules/static-property-placement.md Outdated
Show resolved Hide resolved docs/rules/static-property-placement.md Outdated
Show resolved Hide resolved docs/rules/static-property-placement.md Outdated
Show resolved Hide resolved lib/rules/static-property-placement.js Outdated
Show resolved Hide resolved lib/rules/static-property-placement.js
Show resolved Hide resolved lib/rules/static-property-placement.js Outdated
Show resolved Hide resolved lib/rules/static-property-placement.js Outdated
Show resolved Hide resolved lib/rules/static-property-placement.js Outdated

ljharb and others added some commits Mar 12, 2019

Update docs/rules/static-property-placement.md
Co-Authored-By: dmason30 <danmason@tmcapplications.co.uk>
Update docs/rules/static-property-placement.md
Co-Authored-By: dmason30 <danmason@tmcapplications.co.uk>
Update docs/rules/static-property-placement.md
Co-Authored-By: dmason30 <danmason@tmcapplications.co.uk>
@dmason30

This comment has been minimized.

Copy link
Author

dmason30 commented Mar 12, 2019

Added the static getter option - Great Idea 🎉

I await your feedback.

dmason30 added some commits Mar 12, 2019

@dmason30

This comment has been minimized.

Copy link
Author

dmason30 commented Mar 20, 2019

@ljharb LGTM?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.