Add new rule 'default-props-match-prop-types' #1087

webOS101 · 2017-02-23T10:07:35Z

There's currently one caveat that I haven't addressed: If there are no propTypes at all no warnings are issued. I didn't distinguish between that case and the case where they are not able to be determined (e.g.: external import). Also, this rule may conflict with require-default-props as I am re-using the same component members, though testing did not reveal any problems using the two rules together.

lencioni · 2017-02-24T16:43:26Z

Thanks for the contribution! I haven't had time to review the code here yet, but I wanted to mention that I think no-invalid-default-props isn't quite the right name for this rule. To me that name implies that the default props are going to be checked against the propTypes validators.

I'm not quite sure what a good name might be. Maybe something along the lines of no-extra-default-props?

Also, I'm not entirely sure this makes sense as a separate rule instead of enhancing the prop-types rule to include this. @ljharb what do you think?

ljharb

Your test cases look very thorough!

ljharb · 2017-02-24T16:43:07Z

lib/rules/no-invalid-default-props.js

+    /**
+     * Checks if the Identifier node passed in looks like a defaultProps declaration.
+     * @param   {ASTNode}  node The node to check. Must be an Identifier node.
+     * @returns {Boolean}       `true` if the node is a defaultProps declaration, `false` if not


I don't think any of these spaces are needed; do we typically visually align these comments in other rules?

Happy to make changes, but this code is nearly identical to require-default-props. Do you want me to update the source there, too?

in that case (that our rules already are inconsistent) then don't worry about it; it can be cleaned up in a separate PR.

ljharb · 2017-02-24T16:43:27Z

lib/rules/no-invalid-default-props.js

+     * @returns {Boolean}       `true` if the node is a defaultProps declaration, `false` if not
+     */
+    function isDefaultPropsDeclaration(node) {
+      return (getPropertyName(node) === 'defaultProps' || getPropertyName(node) === 'getDefaultProps');


Let's cache the property name rather than calling the function twice.

ljharb · 2017-02-24T16:45:54Z

lib/rules/no-invalid-default-props.js

+
+        // e.g.:
+        // MyComponent.propTypes.baz = React.PropTypes.string;
+        if (node.parent.type === 'MemberExpression' && node.parent.parent.type === 'AssignmentExpression') {


Will node.parent always have a .parent node?

I think it's unlikely you wouldn't have a parent node in the case of a MemberExpression whose parent is a MemberExpression and belongs to a Component. But, I will admit I'm not intimately familiar with this code.

I don't know much of anything about AST nodes, I just want to avoid a crash in the future when a new node type comes out

ljharb · 2017-02-24T16:46:38Z

lib/rules/no-invalid-default-props.js

+          return;
+        }
+
+        var isPropType = getPropertyName(node) === 'propTypes';


Let's cache the property name here rather than calling the function thrice.

ljharb · 2017-02-24T16:51:41Z

Hmm, that makes sense that "invalid" isn't the best name. extra works, I suppose - or no-superfluous-default-props, or some other synonym?

ljharb · 2017-02-24T16:52:21Z

I do think it belongs as a separate rule since it warns on defaults, not prop types.

webOS101 · 2017-02-24T18:11:20Z

no-unnecessary-default-props? no-unneeded-default-props? no-useless-default-props?

ljharb · 2017-02-24T18:23:45Z

A defaultProp for a required prop is arguably not "useless" since it may serve as documentation (not that I would ever use the option that allows it) so I don't think that's the right word.

unnecessary could work too (along with extra or superfluous).

no-unused-default-props would be good but there's no safe way to tell if the user uses it, only if React will use it.

Let's keep bikeshedding on the name, might as well pick the best one now.

webOS101 · 2017-02-24T18:54:01Z

@lencioni A rule that validates the types of the defaultProps seems very useful!

webOS101 · 2017-02-24T21:52:07Z

I addressed some of the concerns raised in the review, but understand we're still searching for a good name.

webOS101 · 2017-03-02T23:02:37Z

Anyone had any good ideas on the naming? Nothing particularly new has come to me. Maybe no-stray-default-props?

ljharb · 2017-03-31T07:20:15Z

What about default-props-match-prop-types?

webOS101 · 2017-04-04T15:50:31Z

That seems more in keeping with @lencioni 's checker that would validate types. no-undefined-default-props? no-unexpected-default-props? only-meaningful-default-props?

ljharb · 2017-04-04T18:44:50Z

I think default-props-match-prop-types is good, and checking for matching types would be enabled in that rule via an additional option :-)

webOS101 · 2017-04-06T01:13:33Z

I'm curious how accurate we could be in matching the types. Certainly simple types would be easy, more complicated ones could easily become problematic. If you like the new name a lot, I can update the PR to the new name.

ljharb · 2017-04-06T01:26:43Z

I think matching the types will likely only be possible when using the built-in PropTypes validators, or else it'd have to actually pull in and evaluate any custom prop types - but that's a discussion for that separate PR.

I think "match" doesn't necessarily connote "types match", which is why I don't think having the option to match types is a blocker. I mainly just want to get this in, and I think that's a good enough name ¯\_(ツ)_/¯

Fixes jsx-eslint#1022

webOS101 · 2017-04-30T03:03:12Z

Updated the name of the rule and rebased on master.

ljharb

Looks great overall!

ljharb · 2017-04-30T06:47:45Z

lib/rules/default-props-match-prop-types.js

+'use strict';
+
+var has = require('has');
+var find = require('array.prototype.find');


this is no longer necessary, since master now only supports node 4+

What's sad is I originally implemented with Array.find() but realized this didn't match with the (then) current style.

ljharb · 2017-04-30T06:48:06Z

lib/rules/default-props-match-prop-types.js

+     * @returns {ASTNode|null} Return null if the variable could not be found, ASTNode otherwise.
+     */
+    function findVariableByName(name) {
+      var variable = find(variableUtil.variablesInScope(context), function(item) {


so this can be const variable = variableUtil.variablesInScope(context).find(item => item.name === name);

ljharb · 2017-04-30T06:48:10Z

lib/rules/default-props-match-prop-types.js

+     *                                     from this ObjectExpression can't be resolved.
+     */
+    function getDefaultPropsFromObjectExpression(objectExpression) {
+      var hasSpread = find(objectExpression.properties, function(property) {


ljharb · 2017-04-30T06:48:41Z

lib/rules/default-props-match-prop-types.js

+    }
+
+    function propFromName(propTypes, name) {
+      return find(propTypes, function (prop) {


and this can be return propTypes.find(prop => prop.name === name);

Enact-DCO-1.0-Signed-off-by: Roy Sutton roy.sutton@lge.com

webOS101 · 2017-05-01T20:03:34Z

Updated all the var declarations as well.

webOS101 · 2017-05-13T02:01:55Z

Anything else needed for this?

ljharb · 2017-05-17T05:08:43Z

Sorry for the delay. I'll give this another day or so, and if there's no objections, I'll merge it.

webOS101 · 2017-06-05T22:30:28Z

Whoops, looks like I didn't add the rule to the README.md in the root. Should I create a separate PR for that?

ljharb · 2017-06-05T22:31:10Z

@webOS101 yes, thanks, that'd be great!

webOS101 · 2017-06-06T03:55:00Z

#1239

lencioni added enhancement new rule labels Feb 24, 2017

ljharb reviewed Feb 24, 2017

View reviewed changes

Roy Sutton added 6 commits April 29, 2017 22:18

Add new rule 'no-invalid-default-props'

d800461

Fixes jsx-eslint#1022

Find es5

e1055b1

Correct React.createClass defaultProps

cbd90a3

Address review feedback

e05f47a

Update name of rule

d4b6709

Update to createReactClass

0b2c3e2

webOS101 force-pushed the no-invalid-default-props branch from 3118f4d to 0b2c3e2 Compare April 30, 2017 03:02

ljharb changed the title ~~Add new rule 'no-invalid-default-props'~~ Add new rule 'default-props-match-prop-types' Apr 30, 2017

ljharb requested changes Apr 30, 2017

View reviewed changes

Update for Node 4

b69abae

Enact-DCO-1.0-Signed-off-by: Roy Sutton roy.sutton@lge.com

Remove object.assign require

7ec5eb3

ljharb approved these changes May 17, 2017

View reviewed changes

ljharb requested review from yannickcr, lencioni and EvHaus May 17, 2017 05:08

EvHaus approved these changes May 17, 2017

View reviewed changes

ljharb merged commit 62daf82 into jsx-eslint:master May 17, 2017

webOS101 deleted the no-invalid-default-props branch June 6, 2017 04:07

renovate-bot mentioned this pull request Jun 15, 2017

Renovate eslint packages singapore/lint-condo#308

Merged

Add new rule 'default-props-match-prop-types' #1087

Add new rule 'default-props-match-prop-types' #1087

Conversation

webOS101 commented Feb 23, 2017

lencioni commented Feb 24, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Feb 24, 2017

ljharb commented Feb 24, 2017

webOS101 commented Feb 24, 2017

ljharb commented Feb 24, 2017

webOS101 commented Feb 24, 2017

webOS101 commented Feb 24, 2017

webOS101 commented Mar 2, 2017

ljharb commented Mar 31, 2017

webOS101 commented Apr 4, 2017

ljharb commented Apr 4, 2017

webOS101 commented Apr 6, 2017

ljharb commented Apr 6, 2017

webOS101 commented Apr 30, 2017

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webOS101 May 1, 2017 • edited Loading

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

webOS101 commented May 1, 2017

webOS101 commented May 13, 2017

ljharb commented May 17, 2017

webOS101 commented Jun 5, 2017

ljharb commented Jun 5, 2017

webOS101 commented Jun 6, 2017

webOS101 May 1, 2017 •

edited

Loading