[destructuring-assignment] : new rule #1462

DianaSuvorova · 2017-10-03T21:32:34Z

to enforce consistent usage of destructuring assignment closes #216, closes #556, closes #278

ljharb · 2017-10-03T22:34:13Z

docs/rules/destructuring-props-argument.md

+
+## Rule Details
+
+By default rule is set to `always` enforce destructuring assignment. The following pattern is considered warning:


presumably we'd want the default to be "always" on all component types; releasing the rule with only SFC support means we couldn't default class and createClass components to "always" without a major release

I am not sure about your major release concern. Would releasing only SFC support require a major release? What's the problem eith having to do a major release?

Irrespective of that I have only one use case for "other" component types as per issue #278 and I am not particularly keen about it. Are you? Maybe it would make sense to have this rule for SFC only?

releasing only SFC support would be a minor.

Later releasing class-based component supported, defaulted to "ignore", would also be a minor.

However, later releasing class-based component supported, defaulted to "always", would be a major.

There's not a problem per se about releasing a major release, but it's always best to minimize breaking changes.

My use case for non-SFC components is that I want all this.props, this.state, and this.context properties destructured at the top of every function that uses them (that also includes context in SFCs).

It would be great if one rule could handle all of these cases - if not, I'd expect this rule to only ever handle SFCs (props and context), and a new rule to be added for "inside non-SFC functions".

Ok, well in this case I can try to add the rest of the logic to this pr.
So both of the component types will enforce context and props. Class component will in addition enforce state.
Config structure would be: { SFC: always/never/ignore, class: always/never/ignore} (I don't like name class is there a better term?)
default: { SFC: always, class: always }

there is also a nextProps, prevProps which I would ignore for now.
Does this make sense?

i think class is probably the best name, unfortunately.

I agree ignoring next/prev props is fine for now, in SFCs, it'd be props and context, and in class-based and createClass components, it'd be this.props and this.state and this.context.

ljharb · 2017-10-03T22:34:47Z

lib/rules/destructuring-props-argument.js

+        if (relatedComponent && isStatelessFunctionUsage && options.SFC === 'always') {
+          context.report({
+            node: node,
+            message: 'Should use destructuring props assignment in SFC argument'


s/Should/Must/g?

hm, what do you mean?

it's not that you should use it, it's that you must because of the rule

DianaSuvorova · 2017-10-05T16:26:24Z

@ljharb , FYI, this is ready for review.

ljharb

What about things like:

class Foo extends React.Component {
  render() { return this.foo(); }
  foo() {
    return this.props.children; // should be warned when "always"
  }
}

ljharb · 2017-10-05T22:18:43Z

lib/rules/destructuring-assignment.js

+      if (isPropUsed && options.SFC === 'always') {
+        context.report({
+          node: node,
+          message: `Should use destructuring ${node.object.name} assignment`


i think the word "must" here is better than "should"

ljharb · 2017-10-05T22:19:00Z

lib/rules/destructuring-assignment.js

+        if (classComponent) {
+          handleClassUsage(node, classComponent);
+        }
+      }


can we also handle createClassComponents here?

DianaSuvorova · 2017-10-06T16:22:29Z

@ljharb Ready for re-review

class Foo extends React.Component {
  render() { return this.foo(); }
  foo() {
    return this.props.children; // should be warned when "always"
  }
}

var Hello = React.createClass({
      render: function() {
        return <Text>{this.props.foo}</Text>;
      }
    });

Two above examples are added to test cases.

Changed should to must. I am not a big fan of must but I trust your judgement.

ljharb · 2017-10-09T01:41:16Z

In specifications, "should" means the implementation can choose not to comply; "must" means they are forced to comply. If the rule is enabled, they're being forced to comply :-)

ljharb

Can we ensure every example case has a test for both "default", "explicit always", and "explicit never"? In particular, the createClass ones.

I'm also wondering if class-based components and createClass components should have separate configurability?

…destructuring assignment

DianaSuvorova · 2017-10-10T16:10:36Z

I think that never setting doesn't really makes sense for class component. As it would flag any
const { foo } = this.props.
In SFC never is only enforced within the argument.

For example this test is invalid:

{
    code: `const MyComponent = ({ id, className }) => (
      <div id={id} className={className} />
    );`,
    options: [{SFC: 'never'}],
    errors: [
      {message: 'Must never use destructuring props assignment in SFC argument'}
    ]
  }

while this one is valid:

,{
    code: `const MyComponent = (props) => {
      const { id, className } = props;
      return <div id={id} className={className} />
    };`,
    options; [SFC: 'never']
  }

I'm also wondering if class-based components and createClass components should have separate configurability?

I would do it separately if it is a popular request.

I added explicit always test cases (which match default ones). And few more tests for class components. Hope it covers everything.

DianaSuvorova · 2017-10-10T16:20:26Z

@ljharb , btw I am squashing any new commits I add. It may look the last commit was 8days ago but I just pushed the change.

ljharb · 2017-10-11T04:43:24Z

I think that never setting doesn't really makes sense for class component. As it would flag any
const { foo } = this.props.

Yes, that's exactly what I'm expecting - if the user chooses "never", then they want this.props.foo to be used every time.

DianaSuvorova · 2017-10-16T14:46:32Z

@ljharb, this is ready for review

ljharb

This still needs an option and documentation for createClass components.

ljharb · 2017-10-17T05:32:08Z

docs/rules/destructuring-assignment.md

@@ -0,0 +1,87 @@
+# Enforce consostent usage of destructuring assignment of props, state and context (react/destructuring-assignment)
+
+Rule can be set to either of `always`, `never`, `ignore` for `class` and `SFC` components.


this should probably mention createClass components as well

@ljharb, do you suggest createClass as additional option? So it would be [SFC, createClass, class]? If so can it be done as a separate PR? Right now class option includes createClass.

Yes, I think they should be separate options - i think it'd be fine to add it in a separate PR, but I don't think this should be merged with class handling both :-/

hmmm, could you explain rationale behind that? To me this rule almost can be monolithic (cover all possible react components with a single setting). I can see the need for SFC vs Class components distinction, but not class vs createClass. What is the use case?

For one, createClass components are often legacy, and legacy things need different rules. Mainly though, all three are actually distinct constructs - createClass components have different behavior than class-based components, in particular around auto-binding all the "instance" methods;

I think that it'd be fine to allow a string or an object option, where the string option is a single setting for all components - but the object option is intended to allow fine-grained control, so it's supposed to be not monolithic by design.

I agree, technically if we are going to list component types, we should list them all. But even though it is very little code to be added to support the 3rd createClass option, I am not very comfortable adding a special config option for a deprecated functionality. Additionally the difference in behaviour between class and createClass component doesn't seem to be related to what this rule is checking (in both props are referenced with this.props right, or am I wrong?).

So seems like the single string option controlling all components is an acceptable solution for both of us and is common pattern across the library. I will update my pr shortly.

@ljharb I added an incremental change to switch to a single string option so it might be easier to review.

ljharb · 2017-10-17T05:32:21Z

docs/rules/destructuring-assignment.md

+
+Rule can be set to either of `always`, `never`, `ignore` for `class` and `SFC` components.
+```js
+"react/destructuring-assignment": [<enabled>, { "SFC": "always", "class": "always"}]


ljharb · 2017-10-17T05:32:43Z

lib/rules/destructuring-assignment.js

+module.exports = {
+  meta: {
+    docs: {
+      description: 'Enforce consostent usage of destructuring assignment of props, state and context',


s/consostent/consistent

also, please use an Oxford comma after state

hendrikswan · 2017-10-19T14:43:31Z

@DianaSuvorova, thanks for creating this rule - my team is looking forward to when this gets merged in :)

ljharb

lol, that's one way to settle the debate :-p

this is fine; we can add more granular options for the 3 component kinds later :-)

ljharb · 2017-10-22T02:59:12Z

docs/rules/destructuring-assignment.md

@@ -0,0 +1,87 @@
+# Enforce consostent usage of destructuring assignment of props, state and context (react/destructuring-assignment)


consostent → consistent, and please use an Oxford comma after "state"

DianaSuvorova · 2017-10-24T14:14:20Z

@ljharb anything else is missing for this PR?

hendrikswan · 2017-10-24T14:23:10Z

@DianaSuvorova it seems like the changes were approved?

DianaSuvorova · 2017-10-24T15:34:47Z

@hendrikswan, yeah, but I don't have permissions to merge to master.

hendrikswan · 2017-10-24T15:36:22Z

On realized that after my comment.. I admire your perseverance!

…

On Tue, 24 Oct 2017 at 5:34 PM DianaSuvorova ***@***.***> wrote: @hendrikswan <https://github.com/hendrikswan>, yeah, but I don't have permissions to merge to master. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1462 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAQsNeAdBpZiscUk4C1L3ITfAXpwpovSks5svgOZgaJpZM4Psza7> .

DianaSuvorova force-pushed the i216 branch from d01afa8 to 5c7cd7a Compare October 3, 2017 21:45

DianaSuvorova changed the title ~~feat[destructuring-props-argument] : new rule~~ [destructuring-props-argument] : new rule Oct 3, 2017

ljharb reviewed Oct 3, 2017

View reviewed changes

DianaSuvorova force-pushed the i216 branch from 5c7cd7a to 947669f Compare October 4, 2017 20:36

DianaSuvorova changed the title ~~[destructuring-props-argument] : new rule~~ [destructuring-assignment] : new rule Oct 4, 2017

DianaSuvorova force-pushed the i216 branch from 947669f to 3ac88ec Compare October 4, 2017 20:42

ljharb reviewed Oct 5, 2017

View reviewed changes

DianaSuvorova force-pushed the i216 branch 2 times, most recently from 753e3fb to 1a5a9b6 Compare October 6, 2017 16:19

ljharb reviewed Oct 9, 2017

View reviewed changes

[destructuring-assignment] : new rule to enforce consistent usage of …

a25003c

…destructuring assignment

DianaSuvorova force-pushed the i216 branch from 1a5a9b6 to a25003c Compare October 10, 2017 16:12

feat: [destructuring-assignment] setting up never for sfc and class

c967000

ljharb requested changes Oct 17, 2017

View reviewed changes

DianaSuvorova added 2 commits October 19, 2017 11:42

chore(destructuring-assignment) desc typos

1e7dc52

refactor: [destructuring-assignment] a single config option

77b1383

ljharb approved these changes Oct 22, 2017

View reviewed changes

chore:[destructuring-assignment] typos

e80d3b6

dependencies bot mentioned this pull request Mar 1, 2018

Update eslint-plugin-react in / from 7.4.0 to 7.7.0 pcardune/redux-firebase-mirror#17

Open

This was referenced Oct 25, 2018

Update dependency eslint-plugin-react to v7 efcsydney/efcsydney-roster#502

Merged

Update dependency eslint-plugin-react to v7 CaptainGlac1er/StudentHomeworkProject#38

Open

renovate bot mentioned this pull request Nov 7, 2018

chore(deps): update dependency eslint-plugin-react to v7 vgmtv/rsvg#12

Merged

renovate bot mentioned this pull request Nov 23, 2018

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

Open

1 task

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 mesosphere/reactjs-components#475

Closed

1 task


		## Rule Details

		By default rule is set to `always` enforce destructuring assignment. The following pattern is considered warning:

		@@ -0,0 +1,87 @@
		# Enforce consostent usage of destructuring assignment of props, state and context (react/destructuring-assignment)

		Rule can be set to either of `always`, `never`, `ignore` for `class` and `SFC` components.

[destructuring-assignment] : new rule #1462

[destructuring-assignment] : new rule #1462

Conversation

DianaSuvorova commented Oct 3, 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

Choose a reason for hiding this comment

DianaSuvorova commented Oct 5, 2017

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

DianaSuvorova commented Oct 6, 2017 • edited

ljharb commented Oct 9, 2017

ljharb left a comment

Choose a reason for hiding this comment

DianaSuvorova commented Oct 10, 2017 • edited

DianaSuvorova commented Oct 10, 2017

ljharb commented Oct 11, 2017

DianaSuvorova commented Oct 16, 2017

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

DianaSuvorova Oct 21, 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

hendrikswan commented Oct 19, 2017

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DianaSuvorova commented Oct 24, 2017

hendrikswan commented Oct 24, 2017

DianaSuvorova commented Oct 24, 2017

hendrikswan commented Oct 24, 2017 via email

DianaSuvorova commented Oct 3, 2017 •

edited

DianaSuvorova commented Oct 6, 2017 •

edited

DianaSuvorova commented Oct 10, 2017 •

edited

DianaSuvorova Oct 21, 2017 •

edited