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

Jump to bottom

sort-comp Allow methods to belong to any matching group. #1858

Merged
merged 4 commits into from Jul 16, 2018
Merged

sort-comp Allow methods to belong to any matching group. #1858

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2f56e47
sort-comp Allow methods to belong to any matching group.
nosilleg Jun 27, 2018
0b0e462
sort-comp #1858 PR amendments
nosilleg Jun 29, 2018
98ce807
sort-comp #1858 PR amendments
nosilleg Jul 2, 2018
76a4120
sort-comp #1858 PR amendments
nosilleg Jul 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
150 changes: 81 additions & 69 deletions lib/rules/sort-comp.js
View file
Open in desktop
Expand Up @@ -8,6 +8,7 @@ const has = require('has');
const util = require('util');

const Components = require('../util/Components');
const arrayIncludes = require('array-includes');
const astUtil = require('../util/ast');
const docsUrl = require('../util/docsUrl');

Expand Down Expand Up @@ -131,84 +132,91 @@ module.exports = {
* @returns {Array} The matching patterns indexes. Return [Infinity] if there is no match.
*/
function getRefPropIndexes(method) {
let isRegExp;
let matching;
let i;
let j;
const indexes = [];

if (method.getter) {
const getterIndex = methodsOrder.indexOf('getters');
if (getterIndex >= 0) {
indexes.push(getterIndex);
}
}

if (method.setter) {
const setterIndex = methodsOrder.indexOf('setters');
if (setterIndex >= 0) {
indexes.push(setterIndex);
}
}

if (method.typeAnnotation) {
const annotationIndex = methodsOrder.indexOf('type-annotations');
if (annotationIndex >= 0) {
indexes.push(annotationIndex);
}
}
const methodGroupIndexes = [];

if (indexes.length === 0) {
for (i = 0, j = methodsOrder.length; i < j; i++) {
isRegExp = methodsOrder[i].match(regExpRegExp);
if (isRegExp) {
matching = new RegExp(isRegExp[1], isRegExp[2]).test(method.name);
} else {
matching = methodsOrder[i] === method.name;
methodsOrder.forEach((currentGroup, groupIndex) => {
if (currentGroup === 'getters') {
if (method.getter) {
methodGroupIndexes.push(groupIndex);
}
if (matching) {
indexes.push(i);
} else if (currentGroup === 'setters') {
if (method.setter) {
methodGroupIndexes.push(groupIndex);
}
} else if (currentGroup === 'type-annotations') {
if (method.typeAnnotation) {
methodGroupIndexes.push(groupIndex);
}
} else if (currentGroup === 'static-methods') {
if (method.static) {
methodGroupIndexes.push(groupIndex);
}
} else if (currentGroup === 'instance-variables') {
if (method.instanceVariable) {
methodGroupIndexes.push(groupIndex);
}
} else if (currentGroup === 'instance-methods') {
if (method.instanceMethod) {
methodGroupIndexes.push(groupIndex);
}
} else if (arrayIncludes([
'displayName',
'propTypes',
'contextTypes',
'childContextTypes',
'mixins',
'statics',
'defaultProps',
'constructor',
'getDefaultProps',
'state',
'getInitialState',
'getChildContext',
'getDerivedStateFromProps',
'componentWillMount',
'UNSAFE_componentWillMount',
Copy link
Member

Choose a reason for hiding this comment

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

I think the UNSAFE_ methods should only be included in this array when the React version in settings is 16.3+ - same with gDSFP, gSBU, and cDC (for 16.0+)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree with this. In my opinion this makes sense in the config code, but not in the rule implementation itself.

If someone has their config set up so that it includes the UNSAFE_ methods and they have the UNSAFE_ methods in their component then eslint should enforce that config no matter which version of React they are using.

Copy link
Member

Choose a reason for hiding this comment

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

If someone is using React 15, React 16 lifecycle methods should not be sorted in the lifecycle group - because for them, they aren't lifecycle methods. Similarly, if someone is using React 17, where componentWillMount theoretically does not exist, cWM should not be sorted in the lifecycle group - because for them, it's not a lifecycle method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. This is a concern for the code that loads the default config.

This code is dealing with the finalised config. It should not undo a config based on assumptions about the environment. eg the user may have their own UNSAFE_ method that just happens to match the choice a new version of React implements. One would hope this is unlikely, but it would be bad practice to assume it impossible.

Assuming we even wanted to try and undo the config, because custom groups are flattened in the config, and someone could overwrite the lifecycle group, we could only undo methods that were not contiguous with other lifecycle methods. Even then we would be assuming that the user used the lifecycle group when they may have hand coded the list. We could inspect the raw config and deconstruct from there... Why are we trying to undo the users config?

Copy link
Member

Choose a reason for hiding this comment

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

i guess i'm confused why we need a separate list of lifecycle methods, if this is just "the user's config". should this not fall down to line 200?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, line 162 is a functional subset of 200. However 162 is documented behaviour and 200 is not. As stated in the PR description, I've added a test to make sure the functionality isn't lost, but presumed it was better to code to expected behaviours.

Would you prefer that I comment L#200 making it explicit that it's undocumented functionality, or remove 162 and effectively hide (in my opinion) documented behaviour in the code for undocumented behaviour? Of course the tests would catch if someone removed the code thinking they were removing the undocumented behaviour, but that feels too obfuscated to me.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm.

Let's keep this as-is; thanks for your explanation.

'componentDidMount',
'componentWillReceiveProps',
'UNSAFE_componentWillReceiveProps',
'shouldComponentUpdate',
'componentWillUpdate',
'UNSAFE_componentWillUpdate',
'getSnapshotBeforeUpdate',
'componentDidUpdate',
'componentDidCatch',
'componentWillUnmount',
'render'
], currentGroup)) {
if (currentGroup === method.name) {
methodGroupIndexes.push(groupIndex);
}
} else {
// Is the group a regex?
const isRegExp = currentGroup.match(regExpRegExp);
if (isRegExp) {
const isMatching = new RegExp(isRegExp[1], isRegExp[2]).test(method.name);
if (isMatching) {
methodGroupIndexes.push(groupIndex);
}
} else if (currentGroup === method.name) {
methodGroupIndexes.push(groupIndex);
}
}
}

if (indexes.length === 0 && method.static) {
const staticIndex = methodsOrder.indexOf('static-methods');
if (staticIndex >= 0) {
indexes.push(staticIndex);
}
}

if (indexes.length === 0 && method.instanceVariable) {
const annotationIndex = methodsOrder.indexOf('instance-variables');
if (annotationIndex >= 0) {
indexes.push(annotationIndex);
}
}

if (indexes.length === 0 && method.instanceMethod) {
const annotationIndex = methodsOrder.indexOf('instance-methods');
if (annotationIndex >= 0) {
indexes.push(annotationIndex);
}
}
});

// No matching pattern, return 'everything-else' index
if (indexes.length === 0) {
for (i = 0, j = methodsOrder.length; i < j; i++) {
if (methodsOrder[i] === 'everything-else') {
indexes.push(i);
break;
}
}
}
if (methodGroupIndexes.length === 0) {
const everythingElseIndex = methodsOrder.indexOf('everything-else');

// No matching pattern and no 'everything-else' group
if (indexes.length === 0) {
indexes.push(Infinity);
if (everythingElseIndex !== -1) {
methodGroupIndexes.push(everythingElseIndex);
} else {
// No matching pattern and no 'everything-else' group
methodGroupIndexes.push(Infinity);
}
}

return indexes;
return methodGroupIndexes;
}

/**
Expand Down Expand Up @@ -409,6 +417,10 @@ module.exports = {

// Loop around the properties a second time (for comparison)
for (k = 0, l = propertiesInfos.length; k < l; k++) {
if (i === k) {
continue;
}

propB = propertiesInfos[k];

// Compare the properties order
Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Expand Up @@ -24,6 +24,7 @@
"homepage": "https://github.com/yannickcr/eslint-plugin-react",
"bugs": "https://github.com/yannickcr/eslint-plugin-react/issues",
"dependencies": {
"array-includes": "^3.0.3",
"doctrine": "^2.1.0",
"has": "^1.0.3",
"jsx-ast-utils": "^2.0.1",
Expand Down