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.

Sign up for GitHub

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 lifecycle methods check to no-typos rule #1294

Merged
merged 5 commits into from
Jul 30, 2017
Merged

Add lifecycle methods check to no-typos rule #1294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
13fddef
add lifecycle method typos
Jul 11, 2017
968f745
renamed function
Jul 12, 2017
2ee095e
added render method
Jul 12, 2017
75711e5
updated tests
Jul 12, 2017
eaf1ee9
removed isReturningJSX check
Jul 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 41 additions & 3 deletions docs/rules/no-typos.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,32 @@
# Prevents common casing typos (react/no-typos)

Ensure no casing typos were made declaring static class properties
Ensure no casing typos were made declaring static class properties and lifecycle methods.

## Rule Details

This rule checks whether the declared static class properties related to React components
do not contain any typos. It currently makes sure that the following class properties have
This rule checks whether the declared static class properties and lifecycle methods related to React components
do not contain any typos.

It currently makes sure that the following class properties have
no casing typos:

* propTypes
* contextTypes
* childContextTypes
* defaultProps

and the following react lifecycle methods:

* componentWillMount
* componentDidMount
* componentWillReceiveProps
* shouldComponentUpdate
* componentWillUpdate
* componentDidUpdate
* componentWillUnmount
Copy link
Contributor

Choose a reason for hiding this comment

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

What about render? Though React already warns about this when running the code, but still having the error show early is nice.

No `render` method found on the returned component instance: you may have forgotten to define `render`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I agree. I will add render method

* render


The following patterns are considered warnings:

```js
Expand Down Expand Up @@ -47,6 +61,18 @@ class MyComponent extends React.Component {
class MyComponent extends React.Component {
static defaultprops = {}
}

class MyComponent extends React.Component {
componentwillMount() {}
}

class MyComponent extends React.Component {
ComponentWillReceiveProps() {}
}

class MyComponent extends React.Component {
componentdidupdate() {}
}
```

The following patterns are not considered warnings:
Expand All @@ -67,4 +93,16 @@ class MyComponent extends React.Component {
class MyComponent extends React.Component {
static defaultProps = {}
}

class MyComponent extends React.Component {
componentWillMount() {}
}

class MyComponent extends React.Component {
componentWillReceiveProps() {}
}

class MyComponent extends React.Component {
componentDidUpdate() {}
}
```
35 changes: 32 additions & 3 deletions lib/rules/no-typos.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,16 @@ const Components = require('../util/Components');
// ------------------------------------------------------------------------------

const STATIC_CLASS_PROPERTIES = ['propTypes', 'contextTypes', 'childContextTypes', 'defaultProps'];
const LIFECYCLE_METHODS = [
'componentWillMount',
'componentDidMount',
'componentWillReceiveProps',
'shouldComponentUpdate',
'componentWillUpdate',
'componentDidUpdate',
'componentWillUnmount',
'render'
];

module.exports = {
meta: {
Expand All @@ -22,7 +32,7 @@ module.exports = {
},

create: Components.detect((context, components, utils) => {
function reportErrorIfCasingTypo(node, propertyName) {
function reportErrorIfClassPropertyCasingTypo(node, propertyName) {
STATIC_CLASS_PROPERTIES.forEach(CLASS_PROP => {
if (propertyName && CLASS_PROP.toLowerCase() === propertyName.toLowerCase() && CLASS_PROP !== propertyName) {
context.report({
Expand All @@ -33,6 +43,17 @@ module.exports = {
});
}

function reportErrorIfLifecycleMethodCasingTypo(node) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should rename the reportErrorIfCasingTypo function as well to contain something like ifClassProperty.

LIFECYCLE_METHODS.forEach(method => {
if (method.toLowerCase() === node.key.name.toLowerCase() && method !== node.key.name) {
context.report({
node: node,
message: 'Typo in component lifecycle method declaration'
});
}
});
}

return {
ClassProperty: function(node) {
if (!node.static || !utils.isES6Component(node.parent.parent)) {
Expand All @@ -41,7 +62,7 @@ module.exports = {

const tokens = context.getFirstTokens(node, 2);
const propertyName = tokens[1].value;
reportErrorIfCasingTypo(node, propertyName);
reportErrorIfClassPropertyCasingTypo(node, propertyName);
},

MemberExpression: function(node) {
Expand All @@ -52,8 +73,16 @@ module.exports = {
(utils.isES6Component(relatedComponent.node) || utils.isReturningJSX(relatedComponent.node))
) {
const propertyName = node.property.name;
reportErrorIfCasingTypo(node, propertyName);
reportErrorIfClassPropertyCasingTypo(node, propertyName);
}
},

MethodDefinition: function (node) {
if (!utils.isES6Component(node.parent.parent)) {
return;
}

reportErrorIfLifecycleMethodCasingTypo(node);
}
};
})
Expand Down
176 changes: 176 additions & 0 deletions tests/lib/rules/no-typos.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ const parserOptions = {
// -----------------------------------------------------------------------------

const ERROR_MESSAGE = 'Typo in static class property declaration';
const ERROR_MESSAGE_LIFECYCLE_METHOD = 'Typo in component lifecycle method declaration';

const ruleTester = new RuleTester();
ruleTester.run('no-typos', rule, {
Expand Down Expand Up @@ -181,6 +182,64 @@ ruleTester.run('no-typos', rule, {
'First[defautProps] = {};'
].join('\n'),
parserOptions: parserOptions
}, {
code: [
'class Hello extends React.Component {',
' componentWillMount() { }',
' componentDidMount() { }',
' componentWillReceiveProps() { }',
' shouldComponentUpdate() { }',
' componentWillUpdate() { }',
' componentDidUpdate() { }',
' componentWillUnmount() { }',
' render() {',
' return <div>Hello {this.props.name}</div>;',
' }',
'}'
].join('\n'),
parserOptions: parserOptions
}, {
code: [
'class MyClass {',
' componentWillMount() { }',
' componentDidMount() { }',
' componentWillReceiveProps() { }',
' shouldComponentUpdate() { }',
' componentWillUpdate() { }',
' componentDidUpdate() { }',
' componentWillUnmount() { }',
' render() { }',
'}'
].join('\n'),
parserOptions: parserOptions
}, {
code: [
'class MyClass {',
' componentwillmount() { }',
' componentdidmount() { }',
' componentwillreceiveprops() { }',
' shouldcomponentupdate() { }',
' componentwillupdate() { }',
' componentdidupdate() { }',
' componentwillUnmount() { }',
' render() { }',
'}'
].join('\n'),
parserOptions: parserOptions
}, {
code: [
'class MyClass {',
' Componentwillmount() { }',
' Componentdidmount() { }',
' Componentwillreceiveprops() { }',
' Shouldcomponentupdate() { }',
' Componentwillupdate() { }',
' Componentdidupdate() { }',
' ComponentwillUnmount() { }',
' Render() { }',
'}'
].join('\n'),
parserOptions: parserOptions
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we need tests for all lifecycle methods, not just a few.

Additionally, we'll need some valid tests for a class that is not a React component. that can still have any lifecycle method defined with any kind of casing typo.

Lastly, I do not know if we should support this kind of syntax. It seems to work ( https://jsfiddle.net/69z2wepo/82794/ ) but I have never seen anyone do it. @ljharb what do you think of this?

class Hello extends React.Component {
	render() {
  	return <div>Hello {this.props.name}</div>;
  }
}

Hello.prototype.componentDidMount = function() {
  console.log("component has mounted");
}

ReactDOM.render(
  <Hello name="World" />,
  document.getElementById('container')
);

}],

invalid: [{
Expand Down Expand Up @@ -367,5 +426,122 @@ ruleTester.run('no-typos', rule, {
].join('\n'),
parserOptions: parserOptions,
errors: [{message: ERROR_MESSAGE}]
}, {
code: [
'class Hello extends React.Component {',
' ComponentWillMount() { }',
' ComponentDidMount() { }',
' ComponentWillReceiveProps() { }',
' ShouldComponentUpdate() { }',
' ComponentWillUpdate() { }',
' ComponentDidUpdate() { }',
' ComponentWillUnmount() { }',
' render() {',
' return <div>Hello {this.props.name}</div>;',
' }',
'}'
].join('\n'),
parserOptions: parserOptions,
errors: [{
message: ERROR_MESSAGE_LIFECYCLE_METHOD,
type: 'MethodDefinition'
}, {
message: ERROR_MESSAGE_LIFECYCLE_METHOD,
type: 'MethodDefinition'
}, {
message: ERROR_MESSAGE_LIFECYCLE_METHOD,
type: 'MethodDefinition'
}, {
message: ERROR_MESSAGE_LIFECYCLE_METHOD,
type: 'MethodDefinition'
}, {
message: ERROR_MESSAGE_LIFECYCLE_METHOD,
type: 'MethodDefinition'
}, {
message: ERROR_MESSAGE_LIFECYCLE_METHOD,
type: 'MethodDefinition'
}, {
message: ERROR_MESSAGE_LIFECYCLE_METHOD,
type: 'MethodDefinition'
}]
}, {
code: [
'class Hello extends React.Component {',
' Componentwillmount() { }',
' Componentdidmount() { }',
' Componentwillreceiveprops() { }',
' Shouldcomponentupdate() { }',
' Componentwillupdate() { }',
' Componentdidupdate() { }',
' Componentwillunmount() { }',
' Render() {',
' return <div>Hello {this.props.name}</div>;',
' }',
'}'
].join('\n'),
parserOptions: parserOptions,
errors: [{
message: ERROR_MESSAGE_LIFECYCLE_METHOD,
type: 'MethodDefinition'
}, {
message: ERROR_MESSAGE_LIFECYCLE_METHOD,
type: 'MethodDefinition'
}, {
message: ERROR_MESSAGE_LIFECYCLE_METHOD,
type: 'MethodDefinition'
}, {
message: ERROR_MESSAGE_LIFECYCLE_METHOD,
type: 'MethodDefinition'
}, {
message: ERROR_MESSAGE_LIFECYCLE_METHOD,
type: 'MethodDefinition'
}, {
message: ERROR_MESSAGE_LIFECYCLE_METHOD,
type: 'MethodDefinition'
}, {
message: ERROR_MESSAGE_LIFECYCLE_METHOD,
type: 'MethodDefinition'
}, {
message: ERROR_MESSAGE_LIFECYCLE_METHOD,
type: 'MethodDefinition'
}]
}, {
code: [
'class Hello extends React.Component {',
' componentwillmount() { }',
' componentdidmount() { }',
' componentwillreceiveprops() { }',
' shouldcomponentupdate() { }',
' componentwillupdate() { }',
' componentdidupdate() { }',
' componentwillunmount() { }',
' render() {',
' return <div>Hello {this.props.name}</div>;',
' }',
'}'
].join('\n'),
parserOptions: parserOptions,
errors: [{
message: ERROR_MESSAGE_LIFECYCLE_METHOD,
type: 'MethodDefinition'
}, {
message: ERROR_MESSAGE_LIFECYCLE_METHOD,
type: 'MethodDefinition'
}, {
message: ERROR_MESSAGE_LIFECYCLE_METHOD,
type: 'MethodDefinition'
}, {
message: ERROR_MESSAGE_LIFECYCLE_METHOD,
type: 'MethodDefinition'
}, {
message: ERROR_MESSAGE_LIFECYCLE_METHOD,
type: 'MethodDefinition'
}, {
message: ERROR_MESSAGE_LIFECYCLE_METHOD,
type: 'MethodDefinition'
}, {
message: ERROR_MESSAGE_LIFECYCLE_METHOD,
type: 'MethodDefinition'
}]
}]
});