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

nosilleg · 2018-06-27T09:08:57Z

Fixes #1821. Fixes #1838. Fixes #1808. Also fixes a no-issue bug (see bottom in bold).

The history of the rule implies that it was intended that methods matching multiple groups should be able to be placed in any of those groups. See initial commit of the rule.

It seems over time that restrictions have come into the code that prevent this. I didn't find any discussions about this change in behavior. This change has lead to multiple bugs because the config isn't refined enough to accommodate individual preferences.

This PR solves those bugs by reintroducing the ability for methods to be grouped with any matching group.

For example:

Static methods that match a RegExp can be placed with static-methods or with the RegExp group.
Static lifecycle methods can be placed with static-methods or with lifecycle methods.
Static getters can be placed with static-methods or getters.

This allows teams with different preferences to put the method where they think is appropriate, but it means that individuals within a team can still place methods in locations that the team doesn't like and this will need to be a manual check on code review.

This seems like the best compromise until team preferences can be fully supported by the rules options.

About the implementation:

The methodOrder is now processed in array order. This was originally done because I took the approach that the methodOrder should determine stricter behavior. While I no longer believe that to be the solution, I believe this code layout is more readable than previously, and there are fewer loops now than previously. (Previously not all loops were executed, but that would have had to change in this PR.)

I moved the test descriptions into the code blocks, so that when there is a test error you actually know which test has failed. (I didn't see this information otherwise.)

I added two additional tests to cover existing functionality that needed to be verified with the changes in this PR.

The first is that the existing code allows explicitly named functions in the group list. I do not see this in the documentation and there were no tests for it. A decision can be made if this needs to be added to the documentation, or if it's a bug, but the test is there to make sure the behavior isn't accidentally changed.

The second is that method names that match groups names were being forced into those groups. For example, a method named setters would be forced into the setters group. I consider this a bug, and have corrected it in this PR. However, I'm willing to revert to existing behavior if it's deemed appropriate.

ljharb

Thanks, I think this fix is great, and is effectively semver-patch.

ljharb · 2018-06-28T06:36:33Z

lib/rules/sort-comp.js

-            matching = new RegExp(isRegExp[1], isRegExp[2]).test(method.name);
-          } else {
-            matching = methodsOrder[i] === method.name;
+        switch (currentGroup) {


does this have to be a switch? it's so much more verbose than if/elses would be.

It doesn't have to be a switch.

Generally switch is considered to be more readable and more performant than a chain of if/elsees. For me, the potential conciseness of the if/else chain wouldn't help with readability or maintainability; which would be my main concern.

I don't know the performance characteristics for this in JavaScript, but this article would indicate that it's true for JavaScript as well.

The fallthrough block of lifecycle methods is the most verbose part of this. That could be changed to be a part of the default block, which would allow for the shared array of lifecycle method names (mentioned in another comment) with an .includes(currentGroup). I would say that this is less "pure" than the code as it stands, but not enough to block the suggestion.

Of course, you may just want it to be all if/else.

Let me know which option you want to go with.

I definitely disagree; I consider switch to almost always be less readable than an alternative, and certainly less maintainable. It'd be great to go with all if/else.

I will update to be if/else chain.

I would be remiss if I didn't point out that your opinion goes against the popular opinion. e.g. why switch case and not if else if So while this change may be more readable and maintainable for you, it potentially wont be for the masses.

Hi Jordan. It's a shame we have to belabour this point.

I didn't provide JavaScript specific discussions because you didn't specifically mention JavaScript in your opinion. Taking that into consideration is it fair to read your original statement as:

I definitely disagree; I consider switch to almost always be less readable than an alternative, and certainly less maintainable for JavaScript. I agree that it is more readable, maintainable, and performant for other languages. This is the overwhelming shared opinion within the JavaScript community, despite the similarities in syntax between JavaScript and other languages, and the similar performance implications with other languages.

If so, can you please provide references as I'd like to be following community best practices.

Since we're talking about a JS project, the context "in JavaScript" should be assumed. I offer no opinion here on use of switch in languages irrelevant to this repository (namely, everything that's not JavaScript).

Google, as well as common style guides (jslint, for example, since the 90s; airbnb's; etc), can all be consulted for information on that.

https://javascriptweblog.wordpress.com/2010/03/08/caseagainstswitch/

https://ericleads.wordpress.com/2012/12/11/switch-case-considered-harmful/

https://hackernoon.com/rethinking-javascript-eliminate-the-switch-statement-for-better-code-5c81c044716d

https://medium.com/tandemly/whats-wrong-with-the-switch-statement-in-javascript-c560e8ea3c0b

https://codeburst.io/alternative-to-javascripts-switch-statement-with-a-functional-twist-3f572787ba1c
etc.

You mention styleguides, but only link to one person opinion pieces.

The styleguides themselves make no claims that switch should be avoided and instead encourage sane syntax as per every other language that has switch statements.

The opinion pieces that you link to make the main complaints that switch doesn't follow their preferred programming paradigm. I.e. it isn't OO enough (very appropriate for a 2010 opinion piece.), or not functional enough (the new hotness). JavaScript is not a single paradigm language.

You say that JSLint has discourage the use of switch since the 90s, but considering it was created in 2002, and Crockford himself doesn't advocate avoiding switch I'm not sure what to make of your claims.

Others stating that premature deoptimisation is preferable to using using the actual features of a language so that they can pretend it's a different language doesn't seem rational to me. Using JavaScript as JavaScript with the lint enforced best practices does, and the styleguides appear to back this view.

ah lol, you're right about jslint, i misremembered.

Either way, avoiding switch statements is still a best practice (in JavaScript, obviously).

Can you please provide sources that confirm it is best practice to avoid switch statements in JavaScript, or help me understand what is incorrect about my last message if you believe your previous sources were sufficient.

ljharb · 2018-06-28T06:36:48Z

lib/rules/sort-comp.js

-          indexes.push(setterIndex);
-        }
-      }
+      for (let groupIndex = 0; groupIndex < methodsOrder.length; groupIndex++) {


could this be a .map instead of a for loop?

I will update this to be a forEach (since we don't care about the resulting array that a map creates).

I had considered making it a forEach, but reading the code I had assumed that fors were preferable since there's a lot of places that forEach could be used but is not.

ljharb · 2018-06-28T06:37:14Z

lib/rules/sort-comp.js

+            }
+            break;
+          }
+          case 'displayName':


this seems like a list of lifecycle methods that should be a shared constant array, available in multiple rules

I think this is a sane suggestion, but...

It only works if you want to move to one of the two options above where we remove the lifecycle switch block (or switch completely).

And there is code that can make use of the shared array. (After a quick and rudimentary search I'm not seeing any other candidate code, but it may exist.)

ljharb · 2018-07-03T06:11:10Z

lib/rules/sort-comp.js

+          'getChildContext',
+          'getDerivedStateFromProps',
+          'componentWillMount',
+          'UNSAFE_componentWillMount',


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+)

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.

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.

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?

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?

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.

Hmm.

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

ljharb · 2018-07-04T07:45:46Z

lib/rules/sort-comp.js

+          'componentDidCatch',
+          'componentWillUnmount',
+          'render'
+        ].includes(currentGroup)) {


includes is not available on node 4; please use the array-includes package instead of relying on the native method.

ljharb

LGTM, thanks!

nosilleg · 2018-07-13T19:11:10Z

I'm just wanted to calibrate my expectations with this PR. Is there a rough estimate on when it will be merged/released? Is there anything more I can do to help progress it?

ljharb · 2018-12-28T06:58:19Z

v7.12.0 is released.

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

2f56e47

ljharb reviewed Jun 28, 2018

View reviewed changes

ljharb added the bug label Jun 28, 2018

nosilleg added 2 commits June 29, 2018 09:25

sort-comp jsx-eslint#1858 PR amendments

0b0e462

sort-comp jsx-eslint#1858 PR amendments

98ce807

ljharb force-pushed the 1821-sort-comp-order branch 2 times, most recently from d7417ea to 807073c Compare July 3, 2018 06:13

ljharb reviewed Jul 3, 2018

View reviewed changes

ljharb force-pushed the 1821-sort-comp-order branch 2 times, most recently from 5d46d16 to c6bb825 Compare July 3, 2018 06:32

ljharb requested changes Jul 4, 2018

View reviewed changes

sort-comp jsx-eslint#1858 PR amendments

76a4120

nosilleg force-pushed the 1821-sort-comp-order branch from c6bb825 to 76a4120 Compare July 4, 2018 08:51

ljharb approved these changes Jul 5, 2018

View reviewed changes

ljharb requested review from yannickcr, lencioni and EvHaus July 5, 2018 05:05

ljharb merged commit 32b751f into jsx-eslint:master Jul 16, 2018

nosilleg deleted the 1821-sort-comp-order branch July 20, 2018 12:16

metreniuk mentioned this pull request Aug 29, 2018

sort-comp: Impossible order of getDerivedStateFromProps, state and constructor #1839

Closed

renovate bot mentioned this pull request Nov 11, 2018

chore(deps): update dependency eslint-plugin-react to v7.37.2 - autoclosed cheezenaan-sandbox/sample_app_rev4#54

Closed

1 task

This was referenced Nov 23, 2018

chore(deps): update dependency eslint-plugin-react to v7 JLHwung/tianzige#31

Open

Update dependency eslint-plugin-react to v7.11.1 AugurProject/augur#109

Merged

renovate bot mentioned this pull request Dec 5, 2018

Update dependency eslint-plugin-react to v7 Automattic/jetpack#10827

Closed

1 task

mesosphere-web mentioned this pull request Dec 19, 2018

chore(deps): update dependency eslint-plugin-react to v7 d2iq-archive/reactjs-components#475

Closed

1 task

renovate-bot mentioned this pull request Dec 19, 2018

Update dependency eslint-plugin-react to v7.11.1 ampproject/amp-wp#1760

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nosilleg commented Jun 27, 2018 •

edited by ljharb

Loading

ljharb left a comment

ljharb Jun 28, 2018

nosilleg Jun 29, 2018

ljharb Jul 2, 2018

nosilleg Jul 2, 2018

nosilleg Jul 2, 2018

nosilleg Jul 3, 2018

ljharb Jul 3, 2018

nosilleg Jul 4, 2018

ljharb Jul 4, 2018

nosilleg Jul 4, 2018

ljharb Jun 28, 2018

nosilleg Jun 29, 2018

ljharb Jun 28, 2018

nosilleg Jun 29, 2018

ljharb Jul 3, 2018

nosilleg Jul 3, 2018

ljharb Jul 3, 2018

nosilleg Jul 4, 2018

ljharb Jul 4, 2018

nosilleg Jul 4, 2018

ljharb Jul 4, 2018

ljharb Jul 4, 2018

nosilleg Jul 4, 2018

ljharb left a comment

nosilleg commented Jul 13, 2018

ljharb commented Dec 28, 2018

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

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

Conversation

nosilleg commented Jun 27, 2018 • edited by ljharb Loading

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

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

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

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

nosilleg commented Jul 13, 2018

ljharb commented Dec 28, 2018

nosilleg commented Jun 27, 2018 •

edited by ljharb

Loading