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

Add rule to require static propTypes = .. instad of Foo.propTypes = .. for class-based components #1923

Closed
ThiefMaster opened this issue Aug 10, 2018 · 18 comments · Fixed by #2193

Comments

@ThiefMaster
Copy link
Contributor

With such a rule the following would be invalid:

class Foo extends React.Component {}
Foo.propTypes = {...}
Foo.defaultProps = {...}

and this would be valid:

class Foo extends React.Component {
    static propTypes = {...};
    static defaultProps = {...};
}

function Bar() {
    return <>...</>;
}
Bar.propTypes = {...};
@ljharb
Copy link
Member

ljharb commented Aug 10, 2018

Such a rule would need to support both directions: forcing class fields, and forbidding them.

@ThiefMaster
Copy link
Contributor Author

ThiefMaster commented Aug 10, 2018

Agreed.

Would a blacklist/whitelist with property names make sense? I'd say no, since having e.g. defaultProps outside but propTypes inside the class would make no sense.

@ljharb
Copy link
Member

ljharb commented Aug 10, 2018

I’d think you’d set the rule to “static class field” or “outside the class body”, and then separately disable propTypes, defaultProps, contextTypes, etc (ie, set them to “ignore”).

Also, the rule should forbid (or require as a third option) static getter methods for these things.

@alexzherdev
Copy link
Contributor

From what I’ve read here there’s only downsides to using static getters for these, curious why anyone would need to require it?

@ljharb
Copy link
Member

ljharb commented Aug 10, 2018

If someone isn't able to use class fields yet (they're not yet part of the language) and yet still wants to force static properties to go inside of the class body, they'd need to require it.

Additionally, having it be autofixable (ideal), and able to go in all directions, helps migrate a codebase to the inevitable future, static class fields.

@ThiefMaster
Copy link
Contributor Author

If someone isn't able to use class fields yet (they're not yet part of the language)

How likely is it that someone uses react without babel though?

@alexzherdev
Copy link
Contributor

I think the point is that not everyone's babel config includes the class fields transform plugin.

@ljharb
Copy link
Member

ljharb commented Aug 10, 2018

@ThiefMaster "with babel" in no way means "with proposals" - eslint can't parse stage 3 features, for example, and using babel-eslint isn't a great solution because it enables parsing of all features, not just the ones you're transpiling. Separately, airbnb won't use class fields until there's a lint rule that can forbid ever putting functions in them, for reasons I've gone into elsewhere on this repo.

@alexzherdev
Copy link
Contributor

(last comment off topic)

Separately, airbnb won't use class fields until there's a lint rule that can forbid ever putting functions in them, for reasons I've gone into elsewhere on this repo.

That sounds like a fairly straightforward rule, any particular reason it hasn't been implemented?

@ljharb
Copy link
Member

ljharb commented Aug 10, 2018

@alexzherdev it'd belong in eslint core, and they don't support anything til it's stage 4.

@kylemh
Copy link

kylemh commented Oct 23, 2018

I'd love to contribute to this purely to have the opposite rule available for my team. I'll likely poke around this weekend, but was curious what sort of extra work is entailed with making it a "fixable" rule.

I also wanted to know what was meant by getting getters involved into this rule? I was thinking of keeping it simple by either ensuring proptypes are defined within a class, above a class, or a below a class. Should there be a separate rule for how functions are handled?

@ljharb
Copy link
Member

ljharb commented Oct 23, 2018

@kylemh a static-property-placement rule ensuring placement of all of: propTypes, defaultProps, contextTypes, childContextTypes, displayName would be great - the options I'd expect would be "above", "below", "static getters", or "class fields", based on what the ecosystem already does. PRs welcome.

@alexzherdev
Copy link
Contributor

I could be wrong, but isn't sort-comp responsible for ordering of those?

@ljharb
Copy link
Member

ljharb commented Oct 24, 2018

Ordering, yes - but this is about placement and style (sort-comp doesn’t govern things outside the class, for example)

@dmason30
Copy link
Contributor

dmason30 commented Mar 6, 2019

I would also want this for the opposite rule.

As this is the second issue where this has been raised and then no progress.
Duplicate here: #1807

@ljharb Please could this be something you could add in the future?

@ljharb
Copy link
Member

ljharb commented Mar 7, 2019

Closing as a duplicate of #1807.

@dmason30 yes, that's why #1807 has help wanted on it.

@ljharb ljharb closed this as completed Mar 7, 2019
@dmason30
Copy link
Contributor

dmason30 commented Mar 7, 2019

@ljharb I was more politely asking if you could find the time to do the development.

I was able to read the labels thanks. 😞

Sorry if I inconvenienced you.

@ljharb
Copy link
Member

ljharb commented Mar 7, 2019

lol ah. if i can find the time, of course! My time however is very limited.

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

Successfully merging a pull request may close this issue.

5 participants