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

sort-comp: enforcing static lifecycle methods order #1795

Merged
merged 4 commits into from
May 15, 2018
Merged

sort-comp: enforcing static lifecycle methods order #1795

merged 4 commits into from
May 15, 2018

Conversation

lynxtaa
Copy link
Contributor

@lynxtaa lynxtaa commented May 14, 2018

Fixes bug: #1793

Example of incorrect code with "react/sort-comp": "error":

class Test extends React.Component {
	static getDerivedStateFromProps() {
		return null
	}

	constructor(props) {
		super(props)
		this.state = { test: 'test' }
	}

	render() {
		return this.state.test
	}
}

Example of correct code with "react/sort-comp": "error":

class Test extends React.Component {
	constructor(props) {
		super(props)
		this.state = { test: 'test' }
	}

	static getDerivedStateFromProps() {
		return null
	}

	render() {
		return this.state.test
	}
}

@@ -172,7 +172,7 @@ module.exports = {
}
}

if (method.static) {
if (method.static && method.name !== 'getDerivedStateFromProps') {
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to just add indexes.length === 0 or something like that here so all named methods would be caught here rather than just getDerivedStateFromProps for future extensibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, checking for indexes.length === 0 will do the job. Fixed

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.

LGTM, thanks

' constructor() {}',
'}'
].join('\n'),
parser: 'babel-eslint',
Copy link
Member

Choose a reason for hiding this comment

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

the default parser should be sufficient here; please either add a duplicate test (one using default, one using babel-eslint) or, since this is all ES6 syntax, just have the one using the default parser.

@ljharb ljharb added the bug label May 15, 2018
@ThiefMaster
Copy link
Contributor

Does this really make sense? Usually people group static methods together, and put them before the constructor since they are not related to a specific instance.

@ThiefMaster
Copy link
Contributor

Also it's impossible to remove something from the lifecycle list without hardcoding the whole list in my config (and thus having to maintain it whenever react adds new lifecycle methods)...

@edmorley
Copy link

This appears to have caused #1821. Any ideas as to how to fix that case?

@markdemich
Copy link

The way it is now, this is requiring us to put static methods in the middle of other methods. That can't be right, can it? It's breaking all our builds.

@lynxtaa
Copy link
Contributor Author

lynxtaa commented Jun 26, 2018

Static method declarations should be on top but it's not the case for getDerivedStateFromProps.

It was done that way because getDerivedStateFromProps is a lifecycle method (according to current rule description https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/sort-comp.md#rule-details).

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

7 participants