Allow custom PropType classes with no-typos rule #1671

brettdh · 2018-02-01T15:46:15Z

This is simply a rebase of @jseminck's branch, plus the removal of the
lingering failing test.

I removed the failing test because I believe it's a very small corner case
that should not block the resolution of this issue, which seems to affect
a lot of people and limits our ability to keep our components' proptypes DRY.

If it's determined that this corner case should be handled, I suggest creating
a new issue to track just that case.

Closes #1389.

It's a minor corner case, and removing it allows the major issue to be resolved. If it's really necessary to handle that corner case, I suggest creating a new issue to track just that case.

brettdh · 2018-02-01T15:54:24Z

...well, I thought I'd rebased on master. Fixing.

(rebased)

The typos were being ignored, because without `PropTypes`, the un-namespaced identifiers looked like custom proptypes.

brettdh · 2018-02-02T14:13:15Z

Ok, rebased again, conflicts resolved, tests fixed up to handle changes on master. PTAL

ljharb · 2018-02-04T22:58:21Z

Thanks, will review soon.

ljharb

Can we also add tests for using React.PropTypes, and const { PropTypes } = React?

brettdh · 2018-02-09T14:56:52Z

Added them. They fail. 😕 Looking into it.

Added tests for: - `import { PropTypes } from 'react';` - `React.PropTypes`o Fixed a couple subtle bugs that made these new tests fail, and made a small refactoring that made this code easier to understand and fix.

brettdh · 2018-02-09T23:11:20Z

Ok, new tests (and fixes) pushed. Apologies if it's a touch sloppy; I just now got this to work and wanted to get it out before I unplug for the day. PTAL, thanks for the feedback!

ljharb

What about createClass components?

These two tests fail on master as well as this branch, so they expose a pre-existing bug with the no-typos rule.

brettdh · 2018-02-12T13:49:29Z

@ljharb How crucial is that? There are no pre-existing createClass tests for the no-typos rule; adding that coverage strikes me as out of scope here.

Also, there's an implicit test matrix in the no-typos tests, along at least these dimensions:

Method of importing React/PropTypes
Method of declaring component
Method of declaring propTypes object

I wouldn't feel confident about createClass coverage without doing the refactoring to create parameterized tests on these dimensions, which also strikes me as out of scope here.

I created a couple quick-and-dirty createClass versions of other no-typos tests, and they fail when run against master (and also on my branch). So, I'm inclined to think that this is another instance of an issue like #1663, where createClass just isn't supported currently. (I didn't see any other issue related to createClass and no-typos, so I'm guessing it's not something a lot of people are encountering.)

ljharb · 2018-02-19T17:44:24Z

I think it's crucial in that no createClass-related bugs have been reported for it, so even though there's no tests, any change to the rule might break it - and tests ensure that it's not broken.

DRY is generally a bad idea in tests; it'd be better to copy-paste all the needed tests.

However - if, in fact, this rule never worked on createClass components, which your tests seem to suggest, would you be willing to add commented-out createClass tests in this PR, and file a new PR after this one is merged that uncomments the tests? (From that point I can try to get the rule passing, if you don't have the time)

brettdh · 2018-02-19T21:04:04Z

DRY is generally a bad idea in tests; it'd be better to copy-paste all the needed tests.

Strongly disagree on "generally" and definitely in this particular case. The repetition makes it hard to know which are "all the needed tests" - it's hard to see which tests test which behavior, since there is clearly a cross product of behaviors under test. It would be better IMO if that was extracted and made explicit. Thankfully, we don't have to agree on that to make progress with this PR. :)

However - if, in fact, this rule never worked on createClass components, which your tests seem to suggest, would you be willing to add commented-out createClass tests in this PR, and file a new PR after this one is merged that uncomments the tests? (From that point I can try to get the rule passing, if you don't have the time)

Sure, sounds good. I only have a couple such tests, but it'll be a start, anyway. Pushing shortly.

ljharb

LGTM! just two more comments :-)

ljharb · 2018-02-19T22:32:15Z

lib/rules/prefer-stateless-function.js

+        ) {
+          context.report({
+            node: node,
+            message: 'Method returning JSX should be written as stateless function component'


This should probably be pluralized; Methods returning JSX should be written as stateless functional components

ljharb · 2018-02-19T22:33:01Z

lib/rules/prefer-stateless-function.js

@@ -298,6 +302,20 @@ module.exports = {
        markThisAsUsed(node);
      },

+      // Report non-"render" methods that return JSX
+      MethodDefinition: function(node) {


what about computed methods? like ['foo']() { return <div /> } or [Symbol()]() { return <div />; }?

TBH I'm not sure why this commit was in the original branch that I started from here. I'd be fine just removing that commit from this PR, but I'm not sure what would be correct in regards to your comment here.

ah yes, good call. This PR is about the no-typos rule; but it has prefer-stateless-function changes. Let's remove all the parts that aren't about no-typos :-)

brettdh · 2018-02-20T13:21:58Z

Removed prefer-stateless-function commits and rebased on master.

ljharb

Can you add test cases for require as well? we should be able to handle top-level static requires along with import.

Separately, can we make sure that every test using babel-eslint has a duplicate test that uses the default parser?

ljharb · 2018-02-20T18:00:02Z

lib/rules/no-typos.js

+      } else if (callee.type === 'MemberExpression' && callee.property.name === 'oneOfType') {
+        const args = node.arguments[0];
+        if (args && args.type === 'ArrayExpression') {
+          args.elements.forEach(el => checkValidProp(el));


since forEach ignores its return value, this should be an explicit-return arrow function instead.

You're saying you'd prefer args.elements.forEach(el => { checkValidProp(el); });? It seems strictly noisier to me, but I don't feel strongly ¯\_(ツ)_/¯

yep, exactly; implicit return should only be used where the return value matters.

ljharb · 2018-02-20T18:00:42Z

lib/rules/no-typos.js

@@ -54,38 +57,69 @@ module.exports = {
      }
    }

+    function isPropTypesPackage(node) {
+      // Note: we really do want == with the package names here,
+      // since we need value equality, not identity - and


i'm confused about this - every string that has the same contents is the same string by identity, iow 'abc' === 'abc'. Why do we need == here?

I thought that too, but when I was testing, it seemed that only every string literal with the same contents is the same string by identity. I found that was not the case when comparing a literal to a value read from a file.

But maybe there was another issue that I didn't fully understand; I made this jsfiddle to test this, and the results indicate no difference between === and ==, even when comparing a literal to a computed or read-from-external-source value.

I can dig into this more tomorrow, probably.

Perhaps there's invisible characters in the strings? When you run into the difference, try comparing .length (altho then == would fail too). All strings in JS that have the same contents are ===.

Specifically, == is "=== after type coercion", so if they're both typeof "string", == and === are identical, by spec.

WOW ok silly mistake. propTypesPackageName was an array containing one string, which is indeed == to that string value itself, but not ===.

Thanks for not letting that slide. 😳 Pushing fix shortly.

brettdh · 2018-02-21T15:14:14Z

Thanks for your feedback. However, I'm concerned that we're getting solidly into scope creep territory again.

Can you add test cases for require as well? we should be able to handle top-level static requires along with import.

There are no such tests currently; it's not part of this PR to add that coverage. Can we make a separate issue for that?

Separately, can we make sure that every test using babel-eslint has a duplicate test that uses the default parser?

It would be similarly out of scope to ask this for every no-typos test. I assume you mean every new test that this PR adds? Also, this would be a perfect use case for parameterization; it seems quite noisy to copy the entire tests' content just to remove the parser line, and glancing through the tests as they are now, it's quite hard to pick out which tests use which parser. But I feel hesitant to start going down that road, due to the opposition to it you expressed above, and the uncertainty I've expressed about getting it right.

Also, are all tests valid with both parsers? I don't really understand the difference between babel-eslint and the "default parser", so I'm not sure I'm the one to make that change anyway, even if it were in scope here.

ljharb · 2018-02-23T06:43:34Z

I assume you mean every new test that this PR adds?

Yes, certainly only the added tests :-) if the others are missing tests for both the default parser and babel-eslint, that's something we'll have to fix separately.

"Noisy" isn't really a concern in tests; copy-pasting in tests is also fine. The value in testing with both parsers is that each parser might generate different node names, making the rule behave differently (even though it's unlikely). Including both tests prevents any regressions on that in the future as we update eslint and/or babel-eslint.

brettdh · 2018-02-23T13:06:46Z

Added copies of this PR's new tests with the default parser.

brettdh · 2018-03-03T16:50:34Z

Anything else needed on this one? I'd love to get it merged soon.

ljharb · 2018-03-05T06:17:48Z

lib/rules/no-typos.js

+            node.specifiers
+              .filter(specifier => specifier.imported && specifier.imported.name === 'PropTypes')
+              .map(specifier => specifier.local.name)
+          )[0] : null;


should this use .find instead of .filter?

Sure; it makes this code clearer. Updated.

jseminck · 2018-03-11T06:56:08Z

Thanks for dealing with this @brettdh - I also hope to see this merged and released soon 👍

lucaslim · 2018-05-10T03:04:28Z

Any idea when is this going to be released? Or is that a temporary workaround for it until then?

brettdh · 2018-05-10T14:10:22Z

@lucaslim looks like there's an RC now: https://github.com/yannickcr/eslint-plugin-react/releases/tag/v7.8.0-rc.0

yannickcr · 2018-05-10T15:11:19Z

@lucaslim As @brettdh noticed, I published a pre-release a few hours ago.
You can test it using npm i eslint-plugin-react@next -D.
If no major issues are found the final release will be most likely published tomorrow or the day after.

lucaslim · 2018-05-11T05:26:10Z

@yannickcr That's awesome! Thanks :)

jseminck and others added 2 commits November 17, 2017 22:36

Remove unneeded comment

0e1009d

Remove failing test for renamed React import

d318e52

It's a minor corner case, and removing it allows the major issue to be resolved. If it's really necessary to handle that corner case, I suggest creating a new issue to track just that case.

brettdh and others added 3 commits February 1, 2018 12:11

Detect import and require of PropTypes

4abebc3

(rebased)

Fix isRequired tests after custom proptypes changes

ddba876

The typos were being ignored, because without `PropTypes`, the un-namespaced identifiers looked like custom proptypes.

Add react import detection

ec01d9a

brettdh force-pushed the no-typos-react-import branch from b1c1c25 to d91154e Compare February 2, 2018 14:11

ljharb reviewed Feb 9, 2018

View reviewed changes

Add common use-case tests and fix bugs

352642f

Added tests for: - `import { PropTypes } from 'react';` - `React.PropTypes`o Fixed a couple subtle bugs that made these new tests fail, and made a small refactoring that made this code easier to understand and fix.

ljharb requested changes Feb 12, 2018

View reviewed changes

Fix lint errors

830fc3d

brettdh force-pushed the no-typos-react-import branch from cefe6e7 to 7d05324 Compare February 12, 2018 13:38

Add commented-out createClass tests

0f480f6

These two tests fail on master as well as this branch, so they expose a pre-existing bug with the no-typos rule.

brettdh mentioned this pull request Feb 19, 2018

Add podcast series list component goinverse-oss/app#51

Merged

ljharb reviewed Feb 19, 2018

View reviewed changes

brettdh force-pushed the no-typos-react-import branch 2 times, most recently from e91f763 to e9d9990 Compare February 20, 2018 13:21

ljharb reviewed Feb 20, 2018

View reviewed changes

brettdh added 2 commits February 21, 2018 09:43

Fix array-to-string coercion bug

8cc1aa8

Convert value-return arrow function to explicit-return

26c73de

Add default parser tests

3ddfd1c

ljharb approved these changes Mar 5, 2018

View reviewed changes

ljharb reviewed Mar 5, 2018

View reviewed changes

ljharb added the bug label Mar 5, 2018

ljharb requested review from yannickcr, lencioni and EvHaus March 5, 2018 06:18

Use find instead of filter

fc06f85

ljharb force-pushed the no-typos-react-import branch from f21b267 to fc06f85 Compare March 13, 2018 07:39

ljharb merged commit 06ed294 into jsx-eslint:master Mar 13, 2018

ljharb mentioned this pull request Mar 13, 2018

no-typos: Fix failing createClass test #1721

Closed

jseminck mentioned this pull request Mar 25, 2018

react/no-typos false error Typo in declared prop type for Immutable.js ImmutablePropTypes #1741

Closed

brettdh deleted the no-typos-react-import branch May 10, 2018 14:06

rbalicki2 mentioned this pull request May 25, 2018

suggestion: no-typos should have a field for extra propTypes #1802

Closed

jomasti mentioned this pull request Jul 21, 2018

Typo in declared prop type issue #1877

Closed

Allow custom PropType classes with no-typos rule #1671

Allow custom PropType classes with no-typos rule #1671

Conversation

brettdh commented Feb 1, 2018

brettdh commented Feb 1, 2018

brettdh commented Feb 2, 2018

ljharb commented Feb 4, 2018

ljharb left a comment

Choose a reason for hiding this comment

brettdh commented Feb 9, 2018

brettdh commented Feb 9, 2018

ljharb left a comment

Choose a reason for hiding this comment

brettdh commented Feb 12, 2018 • edited Loading

ljharb commented Feb 19, 2018

brettdh commented Feb 19, 2018

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

brettdh commented Feb 20, 2018

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

brettdh Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brettdh commented Feb 21, 2018

ljharb commented Feb 23, 2018

brettdh commented Feb 23, 2018

brettdh commented Mar 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jseminck commented Mar 11, 2018

lucaslim commented May 10, 2018

brettdh commented May 10, 2018

yannickcr commented May 10, 2018

lucaslim commented May 11, 2018

brettdh commented Feb 12, 2018 •

edited

Loading

brettdh Feb 20, 2018 •

edited

Loading