1315925 - Make devtools/client/shared/components pass ESLint's "react/prop-types" rule

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect, P3)

Description

[Julian Descottes [:jdescottes]]

STRs:
- set "react/prop-types" to 2 in devtools/.eslintrc.js
- run ./mach eslint devtools/shared/components

All the validation errors should be fixed.

See https://facebook.github.io/react/docs/typechecking-with-proptypes.html for documentation about typechecking with React and Bug 1264929 for an example of how this was fixed in another module.

Comment 1

[Taylor Alexander Brown]

I would be happy to work on this bug I am unable to replicate it.

I changed the property "react/prop-types" to "2":

    taylor@debian:/media/study/mozilla-central$ hg status
    M devtools/.eslintrc.js
    taylor@debian:/media/study/mozilla-central$ hg diff
    diff --git a/devtools/.eslintrc.js b/devtools/.eslintrc.js
    --- a/devtools/.eslintrc.js
    +++ b/devtools/.eslintrc.js
    @@ -56,8 +56,7 @@ module.exports = {
         "react/no-direct-mutation-state": "error",
         "react/no-unknown-property": "error",
         "react/prefer-es6-class": ["warn", "never"],
    -    // Disabled temporarily until errors are fixed.
    -    "react/prop-types": "off",
    +    "react/prop-types": "2",
         "react/sort-comp": ["error", {
           order: [
             "lifecycle",

And I ran the test without warnings:

    taylor@debian:/media/study/mozilla-central$ ./mach eslint devtools/shared/components
    ✖ 0 problems (0 errors, 0 warnings)

I tested this on a recent build of Firefox Nightly 53.0a1 (2016-12-04) (64-bit):

    taylor@debian:/media/study/mozilla-central$ hg heads
    changeset:   325187:6bdef7ba8b41
    tag:         tip
    parent:      325168:11d06cafe634
    parent:      325186:5144592dd918
    user:        Phil Ringnalda <[e-mail]>
    date:        Sat Dec 03 13:38:35 2016 -0800
    summary:     Merge m-i to m-c, a=merge

If the problem was fixed I would be happy to commit the change.

Let me know if there's something I'm missing.

Comment 2

[Julian Descottes [:jdescottes]]

Sorry, that was a mistake on my part! The actual folder to cleanup is devtools/client/shared/components and not devtools/shared/components. 

Are you still interested to work on this bug?

Comment 3

[Taylor Alexander Brown]

Thank you, I am now able to replicate the bug. 104 errors "'X' is missing in props validation" are reported. I'll see what I can do to fix these and reply if I have any questions.

Comment 4

[Julian Descottes [:jdescottes]]

Thanks! I am assigning the bug to you. 
You can have a look at our contribution guide at https://developer.mozilla.org/en-US/docs/Tools/Contributing if you need help getting started.

You can also take a look at the similar (and already fixed) Bug 1315923 as a reference.

(clearing your needinfo? flag)

Comment 5

[Taylor Alexander Brown]

Attachment: mozilla-central-1315925.patch

Attached is a proposed patch which fixes all of the ESLint errors. I am not all that familiar with this library so I had to make educated guesses based on context.

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8817370 [details] [diff] [review]
mozilla-central-1315925.patch

Sorry I missed your patch ! Adding a review flag so that I don't forget again.

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8817370 [details] [diff] [review]
mozilla-central-1315925.patch

Review of attachment 8817370 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch! And I apologize again for missing it for so long.

Don't hesitate to set a review flag on your attachments or to use the needinfo box on the bottom of the bug page to notify others.

I'm suggesting a few changes below, but otherwise the patch looks great thanks for going through it!
I pushed your changeset + my changes to our continuous integration platform : https://treeherder.mozilla.org/#/jobs?repo=try&revision=263ce07efc17238c2f1e3ac9fb624041ffabd7c8
Our tests will run on various platforms and this page will get updated with the results as they come.

The main added value of running those tests is that they will run in normal & debug mode. In debug mode, devtools are using the development version of react which will check propTypes at runtime.

::: devtools/.eslintrc.js
@@ +56,4 @@
>      "react/no-direct-mutation-state": "error",
>      "react/no-unknown-property": "error",
>      "react/prefer-es6-class": ["warn", "never"],
> +    "react/prop-types": 2,

We can't turn this on just yet, some other components are still breaking the rule.

::: devtools/client/shared/components/reps/array.js
@@ +27,4 @@
>      propTypes: {
>        // @TODO Change this to Object.values once it's supported in Node's version of V8
>        mode: React.PropTypes.oneOf(Object.keys(MODE).map(key => MODE[key])),
> +      objectLink: React.PropTypes.any,

any -> func

::: devtools/client/shared/components/reps/attribute.js
@@ +25,5 @@
>      displayName: "Attr",
>  
>      propTypes: {
> +      object: React.PropTypes.object.isRequired,
> +      objectLink: React.PropTypes.any,

any -> func

::: devtools/client/shared/components/reps/date-time.js
@@ +23,5 @@
>      displayName: "Date",
>  
>      propTypes: {
> +      object: React.PropTypes.object.isRequired,
> +      objectLink: React.PropTypes.any,

any -> func

::: devtools/client/shared/components/reps/document.js
@@ +23,5 @@
>      displayName: "Document",
>  
>      propTypes: {
> +      object: React.PropTypes.object.isRequired,
> +      objectLink: React.PropTypes.any,

any -> func

::: devtools/client/shared/components/reps/element-node.js
@@ +29,5 @@
>        // @TODO Change this to Object.values once it's supported in Node's version of V8
>        mode: React.PropTypes.oneOf(Object.keys(MODE).map(key => MODE[key])),
> +      onDOMNodeMouseOver: React.PropTypes.func,
> +      onDOMNodeMouseOut: React.PropTypes.func,
> +      objectLink: React.PropTypes.any,

any -> func

::: devtools/client/shared/components/reps/error.js
@@ +23,4 @@
>        object: React.PropTypes.object.isRequired,
>        // @TODO Change this to Object.values once it's supported in Node's version of V8
>        mode: React.PropTypes.oneOf(Object.keys(MODE).map(key => MODE[key])),
> +      objectLink: React.PropTypes.any,

any -> func

::: devtools/client/shared/components/reps/function.js
@@ +23,5 @@
>      displayName: "Func",
>  
>      propTypes: {
> +      object: React.PropTypes.object.isRequired,
> +      objectLink: React.PropTypes.any,

any -> func

::: devtools/client/shared/components/reps/grip-array.js
@@ +29,4 @@
>        // @TODO Change this to Object.values once it's supported in Node's version of V8
>        mode: React.PropTypes.oneOf(Object.keys(MODE).map(key => MODE[key])),
>        provider: React.PropTypes.object,
> +      objectLink: React.PropTypes.any,

any -> func

::: devtools/client/shared/components/reps/grip-map.js
@@ +26,4 @@
>        object: React.PropTypes.object,
>        // @TODO Change this to Object.values once it's supported in Node's version of V8
>        mode: React.PropTypes.oneOf(Object.keys(MODE).map(key => MODE[key])),
> +      objectLink: React.PropTypes.any,

any -> func

::: devtools/client/shared/components/reps/grip.js
@@ +31,4 @@
>        mode: React.PropTypes.oneOf(Object.keys(MODE).map(key => MODE[key])),
>        isInterestingProp: React.PropTypes.func,
>        title: React.PropTypes.string,
> +      objectLink: React.PropTypes.any,

any -> func

::: devtools/client/shared/components/reps/long-string.js
@@ +21,5 @@
>      propTypes: {
>        useQuotes: React.PropTypes.bool,
>        style: React.PropTypes.object,
> +      cropLimit: React.PropTypes.number.isRequired,
> +      member: React.PropTypes.string.isRequired,

Looking at the code, member is not required

::: devtools/client/shared/components/reps/object-with-text.js
@@ +24,4 @@
>  
>      propTypes: {
>        object: React.PropTypes.object.isRequired,
> +      objectLink: React.PropTypes.any,

any -> func

::: devtools/client/shared/components/reps/object-with-url.js
@@ +24,4 @@
>  
>      propTypes: {
>        object: React.PropTypes.object.isRequired,
> +      objectLink: React.PropTypes.any,

any -> func

::: devtools/client/shared/components/reps/object.js
@@ +26,4 @@
>        object: React.PropTypes.object,
>        // @TODO Change this to Object.values once it's supported in Node's version of V8
>        mode: React.PropTypes.oneOf(Object.keys(MODE).map(key => MODE[key])),
> +      objectLink: React.PropTypes.any,

any -> func

::: devtools/client/shared/components/reps/promise.js
@@ +26,4 @@
>        object: React.PropTypes.object.isRequired,
>        // @TODO Change this to Object.values once it's supported in Node's version of V8
>        mode: React.PropTypes.oneOf(Object.keys(MODE).map(key => MODE[key])),
> +      objectLink: React.PropTypes.any,

any -> func

::: devtools/client/shared/components/reps/prop-rep.js
@@ +34,4 @@
>        delim: React.PropTypes.string,
>        // @TODO Change this to Object.values once it's supported in Node's version of V8
>        mode: React.PropTypes.oneOf(Object.keys(MODE).map(key => MODE[key])),
> +      objectLink: React.PropTypes.any,

any -> func

::: devtools/client/shared/components/reps/regexp.js
@@ +24,4 @@
>  
>      propTypes: {
>        object: React.PropTypes.object.isRequired,
> +      objectLink: React.PropTypes.any,

any -> func

::: devtools/client/shared/components/reps/string.js
@@ +25,5 @@
>        useQuotes: React.PropTypes.bool,
>        style: React.PropTypes.object,
> +      object: React.PropTypes.string.isRequired,
> +      member: React.PropTypes.any,
> +      cropLimit: React.PropTypes.number.isRequired,

cropLimit is not required

::: devtools/client/shared/components/reps/stylesheet.js
@@ +24,4 @@
>  
>      propTypes: {
>        object: React.PropTypes.object.isRequired,
> +      objectLink: React.PropTypes.any,

any -> func

::: devtools/client/shared/components/reps/text-node.js
@@ +27,4 @@
>        object: React.PropTypes.object.isRequired,
>        // @TODO Change this to Object.values once it's supported in Node's version of V8
>        mode: React.PropTypes.oneOf(Object.keys(MODE).map(key => MODE[key])),
> +      objectLink: React.PropTypes.any,

any -> func

::: devtools/client/shared/components/reps/window.js
@@ +24,4 @@
>  
>      propTypes: {
>        object: React.PropTypes.object.isRequired,
> +      objectLink: React.PropTypes.any,

any -> func

::: devtools/client/shared/components/stack-trace.js
@@ +35,3 @@
>      stacktrace: PropTypes.array.isRequired,
> +    onViewSourceInDebugger: PropTypes.func.isRequired,
> +    asyncCause: PropTypes.string.isRequired,

asyncCause is not a prop for StackTrace

::: devtools/client/shared/components/tree.js
@@ +4,5 @@
>  /* eslint-env browser */
>  "use strict";
>  
> +const React = require("devtools/client/shared/vendor/react");
> +const { DOM: dom, createClass, createFactory, PropTypes } = React;

For the whole file: replace "React.PropTypes" with "PropTypes" since we are directly extracting it from React here.

@@ +631,5 @@
>  const ArrowExpander = createFactory(createClass({
>    displayName: "ArrowExpander",
>  
> +  propTypes: {
> +    item: React.PropTypes.any,

any -> any.isRequired

@@ +632,5 @@
>    displayName: "ArrowExpander",
>  
> +  propTypes: {
> +    item: React.PropTypes.any,
> +    visible: React.PropTypes.any,

any -> bool.isRequired

@@ +633,5 @@
>  
> +  propTypes: {
> +    item: React.PropTypes.any,
> +    visible: React.PropTypes.any,
> +    expanded: React.PropTypes.any,

any -> bool.isRequired

@@ +635,5 @@
> +    item: React.PropTypes.any,
> +    visible: React.PropTypes.any,
> +    expanded: React.PropTypes.any,
> +    onCollapse: React.PropTypes.func.isRequired,
> +    onExpand: React.PropTypes.func.onExpand,

React.PropTypes.func.onExpand -> React.PropTypes.func.isRequired

@@ +678,5 @@
> +    index: React.PropTypes.number.isRequired,
> +    first: React.PropTypes.bool,
> +    last: React.PropTypes.bool,
> +    onFocus: React.PropTypes.func.isRequired,
> +    onBlur: React.PropTypes.func.isRequired,

onBlur & onFocus are not required

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: use-react-dev-to-check-proptypes.patch

You can use this patch in order to get react proptypes errors at runtime with a regular firefox build.

Comment 9

[Taylor Alexander Brown]

Attachment: mozilla-central-1315925-updated.patch

Thank you for the feedback, and no worries about the delay. I've had plenty to keep me busy as I'm sure have you.

I see that the try builds you provided seem to have succeeded.

Attached is an updated patch which implements the changes you suggested. Let me know if there's anything else I've overlooked.

I imported your patch to my working directory without committing it and executed `mach run` and didn't see any errors, however I wasn't sure if I needed to rebuild Firefox for the changes to apply.

Let me know any additional steps I should take to resolve this bug, and thanks again for the help.

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: bug1315925.amended.patch

Thanks! I just rebased your patch on the latest central and removed a "isRequired" in split-box.js, for the style property (looks like it was not needed, and not always provided).

Comment 11

[Julian Descottes [:jdescottes]]

Adding the checkin-needed keyword. A sheriff will now pick up your patch and land it on one of our integration branches. Thanks a lot for fixing all those linting issues! Hopefully we can cleanup the few remaining ones and turn on the eslint validation soon.

Comment 12

[Julian Descottes [:jdescottes]]

Removing checkin-needed for now as I see quite a lot of intermittents on the last try push I did here (https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d3543886272cc9d812a6cb566965190e4b6cc55)

Comment 13

[Julian Descottes [:jdescottes]]

Attachment: bug1315925.amended.v2.patch

Fixed the Number rep propTypes which can actually be number or object.

Comment 14

[Julian Descottes [:jdescottes]]

Attachment: bug1315925.amended.v3.patch

Rebased one last time. Try is green, will land this patch on inbound.

Comment 15

[Julian Descottes [:jdescottes]]

Attachment: bug1315925.amended.v3.patch

wrong head.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cb7b2b0a3ba
Make devtools/client/shared/components pass ESLint's "react/prop-types" rule;r=jdescottes

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2cb7b2b0a3ba

Comment 18

[Taylor Alexander Brown]

My contributions to fix this bug were made for a software engineering course at Oregon State University. A brief report documenting my experiences as a first-time developer can be found here: https://github.com/browtayl/cs362-firefox-developer-tools . Thank you to everyone who helped me contribute to my favorite Web browser while fulfilling my course requirements.

Comment 19

[Julian Descottes [:jdescottes]]

Thank you again for your contribution, and your step-by-step document looks really helpful for first time contributors!