Add rule no static typos #1209

jseminck · 2017-05-19T13:10:54Z

My attempt to add this rule. It seemed pretty basic... so here it is! I am open to new ideas and improvements to the rule. Please let me know!

Issue: #1189

Currently it only checks forstatic class properties. And only for defaultProps, contextTypes and propTypes. It should be trivial to add new properties as well though.

Because it only checks for static properties, it means that having PropTypes as a non-static class property or a method definition is still allowed. Although honestly I do not know why anyone would do that? Perhaps the rule can be extended to also warn in case propTypes is not declared as a static class property?

The rule only works for React Components. Within classes that are not React components, anything is still possible.

ljharb

Thanks, this looks great!

ljharb · 2017-05-19T20:14:23Z

docs/rules/no-static-typos.md

@@ -0,0 +1,59 @@
+# Prevent casing typos when declaring static propTypes, contextType and defaultProps


Please add (react/no-static-typos) on this line.

ljharb · 2017-05-19T20:15:27Z

docs/rules/no-static-typos.md

+
+Ensure no casing typos were made declaring static class properties
+
+**Fixable:** This rule is automatically fixable using the `--fix` flag on the command line.


This shouldn't be fixable - it's not safe to do so.

It's a great rule to prevent people doing this by accident - but autofixing these can cause actual behavior changes in production. eslint core's policy is to never allow autofixes that could break code.

Ok, I will remove it.

I was actually not aware of this policy. So only stylistic issues should be fixable? Adding removing lines, spaces, semi-colons?

FWIW, The actual help for --fix actually says the following: This option instructs ESLint to try to fix as many issues as possible.

Considering the words "as many issues as possible" - I thought that every rule which we know how to fix should be auto fixable.

It's not that it has to just be stylistic - but generally eslint errs on the side of caution to avoid breaking code.

If this rule warns, it'll only be for two reasons: 1) a typo, which is totally autofixable - and 2) a badly but intentionally chosen name, and the user needs to know to manually rename it.

ljharb · 2017-05-19T20:15:43Z

docs/rules/no-static-typos.md

+
+* propTypes
+* contextTypes
+* defaultProps


what about childContextTypes?

jseminck · 2017-05-23T12:36:55Z

Tackled your comments. I was not aware of childContextTypes - it is not something I have had to use before. If there are any properties that should be added please let me know. It's pretty trivial to add them - we just have to make sure the test cases are added as well.

Also I wanted to ask: as I was working on this, I thought that it might be worth it to detect small spelling mistakes as well. We should be able to easily detect 1-letter spelling mistakes. But I am not sure how far this plugin wants to go?

ljharb · 2017-05-23T20:11:13Z

Detecting spelling mistakes seems like a slippery slope.

ljharb

For each test case (that are currently all using babel-eslint), can you please add a duplicate test case that uses the default parser?

jseminck · 2017-05-31T04:56:06Z

The default parser throws an error. I assume that is the case because the class properties proposal is still stage-0 and it is not supported by espree (which is the default parser).

This is the error that the test then gives: https://astexplorer.net/#/gist/da19b693a7a3d48bcc3fb249180b5295/a5875bd2c21133f64ce9f397ff82adb5e37da26a

Please correct me if I am wrong, would need guidance what exactly needs to be changed. :)

Edit: Did you mean that the following should also be validated? Because currently this is not checked and does not throw a warning.

class Hello extends Component [}
Hello.pRopTyPeS = { ... } // should throw an error because of casing issues?

I did remove the require("babel-eslint"); at the top of the test file. That seems like it was not needed.

ljharb · 2017-05-31T05:24:53Z

Indeed, I'm talking about assigning it outside the class - this should apply to the currently-standard Foo.propTypes = {} as well as the not-yet-standard class properties form.

jseminck · 2017-06-05T05:22:58Z

I added tests and support for assignment outside the class.

But I cannot get it to work when assigning on a stateless function component: I am not sure how to detect whether the function is a stateless function component or just some random function yet.

      function MyComponent(props) {
        return <div>{props.name}</div>;
      }

      QuestionList.PrOpTyPeS = {
        name: PropTypes.string.isRequired,
      };

ljharb · 2017-06-05T20:36:00Z

We have Components.detect used elsewhere in the project which should be able to handle that for you.

jseminck · 2017-06-13T05:13:36Z

I spent quite a bit of time on this but no luck so far. I cannot figure out how to properly detect a stateless function component so that the following becomes invalid:

function MyComponent() { return (<div>{this.props.myprop}</div>) };
MyComponent.pRoPTyPeS = { myProp: PropTypes.string }

But the following is valid (since it is not a stateless component):

function SomeRandomFunction() { return {} };
MyComponent.pRoPTyPeS = { }

I tried using components.isReturningJSX but didn't get anywhere.

I added all the test cases, maybe someone else has any idea?

ljharb · 2017-06-13T05:50:10Z

Components.detect?

jseminck · 2017-06-24T17:06:50Z

So I finally managed to fix this. I also rebased everything unto the latest master.

I ended up extending Components.findReturnStatement to also work when passing in FunctionDeclaration node. Previously, it would not work for this specific node.

This way, I could now use utils.isReturningJSX to properly detect a functional stateless component compared to a normal function..

I tried various other approaches from the existing Components API but nothing wold make the tests pass as I expected them to.

lencioni · 2017-06-26T18:07:21Z

README.md

@@ -100,6 +100,7 @@ Finally, enable all of the rules that you would like to use.  Use [our preset](#
 * [react/no-redundant-should-component-update](docs/rules/no-redundant-should-component-update.md): Prevent usage of `shouldComponentUpdate` when extending React.PureComponent
 * [react/no-render-return-value](docs/rules/no-render-return-value.md): Prevent usage of the return value of `React.render`
 * [react/no-set-state](docs/rules/no-set-state.md): Prevent usage of `setState`
+* [react/no-static-typos](docs/rules/no-static-typos.md): Prevent casing typos when declaring static class properties.


In addition to casing, it would be nice to flag also anything with a small levenshtein distance. That could be done as a separate PR.

And, if you are interested, it would be really cool to have a similar rule for detecting typos in lifecycle methods: #843

I wonder if this rule should have a more general name like react/no-typos and make it work also for lifecycle methods?

I suggested the spelling error detection also earlier in the PR: #1209 (comment) and reply: #1209 (comment)

I will look into the similar issue regarding lifecycle methods. Would be useful to have it there as well, as I never remember if it was prevProps, nextProps, prevState or nextState (which one and in which order).

I think the rename would make sense, if we add support for lifecycle methods... which I think, should be straight forward. (though I though that initially about this rule as well and it turned out to take a bit of time :D)

as I never remember if it was prevProps, nextProps, prevState or nextState

To be clear, my suggestion is to apply this rule to lifecycle method names, not their arguments. e.g. componentWillMount. A rule that enforces naming of lifecycle method arguments would be interesting, but I think that is definitely a different rule than typo detection.

Even if we're planning on extending it in the future but not yet, then we should rename the rule to be more generic.

Agreed. I will rename to no-typos and update the docs accordingly.

We can then add casing typos for lifecycle methods in another PR. And we can look into detecting small Levenshtein distances in a separate PR as well.

Sounds good?

Sounds great!

lencioni · 2017-06-26T18:10:40Z

lib/rules/no-static-typos.js

+        if (CLASS_PROP.toLowerCase() === propertyName.toLowerCase() && CLASS_PROP !== propertyName) {
+          context.report({
+            node: node,
+            message: 'Typo in static class property declaration'


It seems like this might be pretty straightforward to write a fixer for. Would you be interested in adding that?

I don't think a fixer is safe here; there's no way to know if it's intentional or not.

Fix an error with closing the code tag Add "fixable" to the documentation Add link to the main README

Added the rule name Decided to not mention all the possible properties that we check for in the title, as adding childContextTypes in the next commit may end up making this title too long.

findReturnStatement expect "node.value.body.body" to exist, but for FunctionDeclarations the correct path to the ReturnStatement is "node.body.body"

ljharb

We should also add tests for computed static property names (to ensure they're not reported).

jseminck · 2017-06-29T07:19:44Z

Could you please elaborate a bit more what you mean exactly?

I added a test case for this, as this was currently causing a crash:

      class First extends React.Component {}
      First["prop" + "Types"] = {};'
      First["context" + "Types"] = {};'
      First["childContext" + "Types"] = {};'
      First["default" + "Props"] = {};

And yeah - this is not supported. E.g. First["deFauLT" + "prOps"] = {} will not error.

ljharb · 2017-06-29T07:33:03Z

I expect, from the implementation, that computed property names are ignored. I'm asking for tests to ensure they are ignored - to prevent them accidentally being considered in the future, and to ensure that considering them is a breaking change.

jseminck · 2017-07-01T08:21:26Z

I understood why we want the test cases. I just wasn't 100% sure that I understood correctly what you meant with "computed static property names".

I understood this was used for:

const myObj = {
  ["myKey-" + getIndex()]: "myValue"
}

But I don't see how this syntax can be used to declare these properties. The tests I added are basically what this would transpile to:

const myObj = {}
myObj["myKey-" + getIndex()]: "myValue

I hope that's all that is needed to finalize this, as I'd like to start focussing on the next steps. :)

ljharb requested changes May 19, 2017

View reviewed changes

ljharb added the new rule label May 19, 2017

jseminck force-pushed the rule-no-static-typos branch from 9ad59da to 3aa552c Compare May 29, 2017 12:08

ljharb requested changes May 31, 2017

View reviewed changes

jseminck changed the title ~~Add rule no static typos (including fixer)~~ Add rule no static typos May 31, 2017

jseminck force-pushed the rule-no-static-typos branch from a487c4f to 3804f34 Compare June 24, 2017 16:44

lencioni approved these changes Jun 26, 2017

View reviewed changes

lencioni reviewed Jun 26, 2017

View reviewed changes

Joachim Seminck added 13 commits June 28, 2017 09:29

Add no-static-typos rule

345aef4

Add fix for no-static-typos

860d7f3

Fix documentation:

6a0a614

Fix an error with closing the code tag Add "fixable" to the documentation Add link to the main README

Another minor documentation fix

692a94f

Remove fixer from no-static-typos

89f340c

Update the title for the readme for the file

36fae24

Added the rule name Decided to not mention all the possible properties that we check for in the title, as adding childContextTypes in the next commit may end up making this title too long.

Add support for childContextTypes

4a596ba

Remove requiring babel-eslint in the test

01d4338

Also handle cases where we declare propTypes outside of the class

394b4d5

Add test cases for function components

5ac830e

Fix syntax errors in tests

ccff8ea

Add suport for FunctionDeclaration to findReturnStatement

faea2af

findReturnStatement expect "node.value.body.body" to exist, but for FunctionDeclarations the correct path to the ReturnStatement is "node.body.body"

Properly detect ES6 components and Stateless functional components

49a7dfa

Joachim Seminck added 5 commits June 28, 2017 09:30

const to let

3e68929

Fix linting errors that popped up after rebase on master

ebd4231

Fix another lint error that doesn't show up locally, only on travis :o

3c3baef

var to const

7620d47

Rename no-static-typos to no-typos

efb7366

jseminck force-pushed the rule-no-static-typos branch from 27fc5bc to 84bd235 Compare June 28, 2017 07:31

Fix linting errors from the var -> const/let PR

84bd235

ljharb approved these changes Jun 28, 2017

View reviewed changes

Add test case that was causing a crash

73d4941

Joachim Seminck added 2 commits July 1, 2017 09:27

Add another valid test case

82facc8

Add another valid test case

afd6ea0

ljharb merged commit 690832f into jsx-eslint:master Jul 1, 2017

ljharb mentioned this pull request Jul 17, 2017

Add no-common-typos rule #981

Closed

1 task

ljharb mentioned this pull request Aug 14, 2017

Rule to prevent static PropTypes #1189

Closed

ljharb mentioned this pull request Feb 6, 2022

[New Rule] Check propTypes are static #1896

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rule no static typos #1209

Add rule no static typos #1209

jseminck commented May 19, 2017 •

edited

ljharb left a comment

ljharb May 19, 2017

ljharb May 19, 2017

jseminck May 20, 2017 •

edited

ljharb May 20, 2017

ljharb May 19, 2017

jseminck commented May 23, 2017 •

edited

ljharb commented May 23, 2017

ljharb left a comment

jseminck commented May 31, 2017 •

edited

ljharb commented May 31, 2017

jseminck commented Jun 5, 2017

ljharb commented Jun 5, 2017

jseminck commented Jun 13, 2017

ljharb commented Jun 13, 2017

jseminck commented Jun 24, 2017

lencioni Jun 26, 2017

jseminck Jun 26, 2017 •

edited

lencioni Jun 26, 2017

ljharb Jun 26, 2017

jseminck Jun 27, 2017

ljharb Jun 27, 2017

lencioni Jun 26, 2017

ljharb Jun 26, 2017

ljharb left a comment

jseminck commented Jun 29, 2017

ljharb commented Jun 29, 2017

jseminck commented Jul 1, 2017

		@@ -0,0 +1,59 @@
		# Prevent casing typos when declaring static propTypes, contextType and defaultProps


		Ensure no casing typos were made declaring static class properties

		Fixable: This rule is automatically fixable using the `--fix` flag on the command line.

Add rule no static typos #1209

Add rule no static typos #1209

Conversation

jseminck commented May 19, 2017 • edited

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jseminck May 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jseminck commented May 23, 2017 • edited

ljharb commented May 23, 2017

ljharb left a comment

Choose a reason for hiding this comment

jseminck commented May 31, 2017 • edited

ljharb commented May 31, 2017

jseminck commented Jun 5, 2017

ljharb commented Jun 5, 2017

jseminck commented Jun 13, 2017

ljharb commented Jun 13, 2017

jseminck commented Jun 24, 2017

Choose a reason for hiding this comment

jseminck Jun 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb left a comment

Choose a reason for hiding this comment

jseminck commented Jun 29, 2017

ljharb commented Jun 29, 2017

jseminck commented Jul 1, 2017

jseminck commented May 19, 2017 •

edited

jseminck May 20, 2017 •

edited

jseminck commented May 23, 2017 •

edited

jseminck commented May 31, 2017 •

edited

jseminck Jun 26, 2017 •

edited