1264929 - Make about:debugging pass ESLint's "react/prop-types" rule.

Status: RESOLVED FIXED

(DevTools :: about:debugging, defect, P3)

Description

[Jan Keromnes [:janx]]

A while ago, bug 1245496 fixed all ESLint errors for about:debugging, but since then new rules have been added and about:debugging needs fixing again.

Most notably, the new rule "react/prop-types" requires all React components to validate their props, by clearly specifying the expected types of each prop[0].

The goal of this bug is to change "react/prop-types" to `2` (error level) in /devtools/.eslintrc[1], and fix all the resulting errors generated by the following command:

    $ ./mach eslint devtools/client/aboutdebugging/

- A list of all the current failures found by this command can be found in bug 1251271 comment 2.
- Note: You need to run `./mach eslint --setup` first if you haven't done it before.
- Bonus points for fixing similar errors found in other devtools modules, e.g. /devtools/client/responsive.html/.

I am mentoring this bug, so if you're interested in fixing it, or if you have any question, please comment on the bug and set the "needinfo?" flag to ":janx".

[0] https://facebook.github.io/react/docs/reusable-components.html#prop-validation
[1] https://github.com/mozilla/gecko-dev/blob/master/devtools/.eslintrc#L50

Comment 1

[Feng Guo]

Hi Jan, Can I work on this bug? thanks

Comment 2

[Feng Guo]

(In reply to Jan Keromnes [:janx] from comment #0)
> A while ago, bug 1245496 fixed all ESLint errors for about:debugging, but
> since then new rules have been added and about:debugging needs fixing again.
> 
> Most notably, the new rule "react/prop-types" requires all React components
> to validate their props, by clearly specifying the expected types of each
> prop[0].
> 
> The goal of this bug is to change "react/prop-types" to `2` (error level) in
> /devtools/.eslintrc[1], and fix all the resulting errors generated by the
> following command:
> 
>     $ ./mach eslint devtools/client/aboutdebugging/
> 
> - A list of all the current failures found by this command can be found in
> bug 1251271 comment 2.
> - Note: You need to run `./mach eslint --setup` first if you haven't done it
> before.
> - Bonus points for fixing similar errors found in other devtools modules,
> e.g. /devtools/client/responsive.html/.
> 
> I am mentoring this bug, so if you're interested in fixing it, or if you
> have any question, please comment on the bug and set the "needinfo?" flag to
> ":janx".
> 
> [0]
> https://facebook.github.io/react/docs/reusable-components.html#prop-
> validation
> [1] https://github.com/mozilla/gecko-dev/blob/master/devtools/.eslintrc#L50

I run the setup and the command it shows no error encounter? 


 0:00.13 Running /usr/bin/eslint
 0:00.13 /usr/bin/eslint --plugin html --ext [.js,.jsm,.jsx,.xml,.html] devtools/client/aboutdebugging/
 0:02.85 Finished eslint. No errors encountered.

Comment 3

[Jan Keromnes [:janx]]

(In reply to Feng Guo from comment #1)
> Hi Jan, Can I work on this bug? thanks

Hi Feng Guo, for sure! Thanks for your interest, I'm assigning this bug to you.

(In reply to Feng Guo from comment #2)
> I run the setup and the command it shows no error encounter? 
> 
> 
>  0:00.13 Running /usr/bin/eslint
>  0:00.13 /usr/bin/eslint --plugin html --ext [.js,.jsm,.jsx,.xml,.html]
> devtools/client/aboutdebugging/
>  0:02.85 Finished eslint. No errors encountered.

You're right, we seem to have a bug with ESLint currently, preventing it from recursing into directories.

To work around this, you can run:

    ./mach eslint devtools/client/aboutdebugging/*
    ./mach eslint devtools/client/aboutdebugging/components/*
    ./mach eslint devtools/client/aboutdebugging/modules/*
    ./mach eslint devtools/client/aboutdebugging/test/*

In the meantime, I'll try to find out where that problem comes from.

Comment 4

[Jan Keromnes [:janx]]

(Setting the needinfo? flag on you so that you're aware that I replied.)

Comment 5

[Jan Keromnes [:janx]]

(Also, I can help, please ask any questions if something isn't clear.)

Comment 6

[Feng Guo]

(In reply to Jan Keromnes [:janx] from comment #5)
> (Also, I can help, please ask any questions if something isn't clear.)

Hi Jan, after change prop-type to 2 and I should focus on checking the error in .eslintrc file right? Thanks

Comment 7

[Feng Guo]

Hi Jan,

By checking the error do I have to change the js files in aboutdebugging  or change eslintrc to pass the test?

Comment 8

[Jan Keromnes [:janx]]

Hi Feng Guo,

(In reply to Feng Guo from comment #7)
> By checking the error do I have to change the js files in aboutdebugging  or
> change eslintrc to pass the test?

If possible, please fix the JS files in /devtools/client/aboutdebugging/ to pass the test.

For example, in /devtools/client/aboutdebugging/components/service-worker-target.js you could add a `propTypes` attribute to the `ServiceWorkerTarget` React class, and it will probably look like this:

  propTypes: {
    client: function(props, propName, componentName) {
      if (!(props[propName] instanceof DebuggerClient)) {
        return new Error(componentName + " expects prop `" + propName +
          "` to be an instance of `DebuggerClient`.");
        );
      }
    },
    debugDisabled: PropTypes.bool,
    target: PropTypes.shape({
      icon: PropTypes.string,
      name: PropTypes.string.isRequired,
      url: PropTypes.string,
      scope: PropTypes.string,
      registrationActor: PropTypes.string.isRequired,
      workerActor: PropTypes.string
    })
  }

These are all the props used in the `ServiceWorkerTarget` class. While the `client` will be the same in other React classes, the `target` prop shape can slightly vary. Please let me know if you're unsure what propTypes a particular class should specify.

You can get `DebuggerClient` by adding the following line to the top of the file:

  loader.lazyRequireGetter(this, "DebuggerClient",
    "devtools/shared/client/main", true);

And also `PropTypes` by changing an existing line like so:

  const { createClass, DOM: dom, PropTypes } =
    require("devtools/client/shared/vendor/react");

Comment 9

[Jan Keromnes [:janx]]

Hi Feng Guo, are you still interested in working on this bug?

- If you are, we can keep the bug assigned to you. Please ask any questions if something isn't clear, or if you'd like some help.

- If you don't have time to look into this anymore, maybe we can free this bug for someone else that might be interested in trying to fix it?

Comment 10

[Jan Keromnes [:janx]]

Feng Guo, thanks again for offering your help, and for starting to investigate this bug. You have already helped us get closer to fixing it! Sorry that it turned out a bit more complicated than anticipated, but it's not a problem that you didn't have more time to spend on it. If one day you'd like to try again, please feel free to take any unassigned bug, and to ask any questions. We're thankful for any help we can get, and we're more than happy to mentor anyone interested.

Patrick Pei reached out to me via email, and is interested in taking it from here. Thank you Patrick!

Comment 11

[patrickkerry.pei]

Hi,

In regard to the node setup, it can find the binary from a git bash client but not from within the /c/mozilla-source/mozilla-central directory.. is there a way to reconfigure this?

Comment 12

[Jan Keromnes [:janx]]

Hi Patrick, sorry for the delayed response, I was away for a week.

I guess from your email that you're on Windows. For reference, the problem you're seeing is a "node: command not found" error when trying to run ESLint with "./mach eslint devtools/client/aboutdebugging/".

From comment 11, I understand that you're able to run a Git Bash client, where the command `node` is defined (i.e. you can run the `node` command and see the "> " prompt, which means the Node.js install dir is present somewhere in your "$PATH" environment variable). What is the result of `which node` in your Git Bash client?

However, once you're in "/c/mozilla-source/mozilla-central", it looks like your terminal can't find the `node` binary anymore. Are you using the same Git Bash client to run the "./mach eslint" command? If not, could you please try that?

Another, easier way to access a Firefox checkout with ESLint already pre-configured is to use a cloud environment on https://janitor.technology (it's currently in Alpha and invite-only, but if you're interested I can invite you).

Comment 13

[Jan Keromnes [:janx]]

Attachment: mozilla-config.PNG

Uploading a screenshot Patrick sent me via email for future reference.

Here is my response email:

---

> There's no way to post pictures in the forum

Actually there is a way: you can click on "Add an attachment" and upload a picture there. [Note: you have to click on "attach a file" if there is no "Browse..." button.]

> I'm sorry about using the git BASH client before and complicating things; I'll run everything from native command line from now on.

No worries, I guess the complication here is Windows, not which bash client you use. If Git Bash works for you, that's great! :)

> (I don't even think it can find the mozilla-native 'mach' command - probably because of the node setup?)

Finding `./mach` shouldn't be related to the `node` setup. Also, from your screenshot, it looks like `./mach` works as expected, but it can't find the `eslint` binary in "/c/mozilla-source/mozilla-central/testing/eslint". You'd need to run `./mach eslint --setup` in the left terminal, but from your `which node` result, I guess that won't work because you don't have access to `node`.

However, the terminal on the right seems to work fine (because `which node` is able to find the `node` binary). What happens if, in that terminal, you run the following commands?

    cd ../../mozilla-source/mozilla-central/
    ./mach eslint --setup

If all this still doesn't work, I have another solution for you: The website https://janitor.technology lets you access a Firefox checkout inside Cloud9 IDE (https://c9.io) with `eslint` already pre-configured. This should be a lot easier to setup and use to write a patch.

If you have any more questions, please ask away, if I can help you get started on this patch I'll be very happy.

Comment 14

[patrickkerry.pei]

Attachment: mozilla-config2.PNG

Comment 15

[patrickkerry.pei]

I think it's now telling me I don't have python 2.7 even though I have it installed already.. You mentioned that the website https://janitor.technology is invite only - could I have an invite to that please?

Comment 16

[patrickkerry.pei]

Letting you know I replied - thanks!

Comment 17

[Jan Keromnes [:janx]]

Hi Patrick, thanks for using the needinfo flag!

Your second attachment seems to show the same problem you had in the left terminal window of the first attachment... I don't really know what to do at this point.

Trying the website https://janitor.technology should be much easier to get you started:

1. You should have received an email called "Janitor Invite", with instructions on how to access your account.

1'. If you can't find that email, please sign in at https://janitor.technology/login with your email address, and you should receive an email called "Janitor Sign-in link".

2. Once you've signed in, please make sure you created a free Cloud9 account at https://c9.io, then head to https://janitor.technology/account/ and add your Cloud9 username and your Cloud9 SSH public key (see instructions in the "Janitor Invite" email, or directly on the "Account" page). Sorry, the set-up is a bit messy still, but it will get much easier/quicker soon.

3. Once you've done this, you can click on "Edit in Cloud9" next to the Firefox project in https://janitor.technology/projects/.

4. Then in Cloud9's terminal, you'll be able to run `./mach eslint devtools/client/aboutdebugging/` without issues.

Please let me know if you have any other question or problem.

Comment 18

[patrickkerry.pei]

I'm sorry for more issues but after clicking "Edit in Cloud9" next to the Firefox project, it takes me to a page prompting to create a new workspace and I'm assuming the preferred option is to create a Remote SSH workspace but that's "a premium feature". 

If you meant for me to create a hosted workspace, there's a clone from git/mercurial URL optional input box but trying to clone from 'https://hg.mozilla.org/' and running 'ls' in the bash client in my workspace shows that it's completely empty..

Thanks again!

Comment 19

[Jan Keromnes [:janx]]

Indeed SSH workspaces are usually a premium feature, but Cloud9 is very generously sponsoring Janitor Alpha accounts. It just takes some time to process your invitation, and when you tried earlier, the link between your Janitor and Cloud9 accounts wasn't yet established. I'm happy to confirm that this has been fixed now.

TL;DR - everything should work now, please try again.

Comment 20

[patrickkerry.pei]

I think I finished fixing all the errors but have a question about the 'target' prop that originates from the client (DebuggerClient)'s call to listAddons(), which subsequently makes a call to the DebuggerClient's requester function...(this is all in ./devtools/shared/main.js) Is there somewhere I can find the documentation for the return object?

Also, I noticed that in comment 8, you listed the target actors as string props - is there a reason for that? I thought that https://developer.mozilla.org/en-US/Add-ons/SDK/Low-Level_APIs/dev_panel#Actors indicated that each Actor is a Vulcan.js object?

Lastly, does the process of submitting a patch differ at all from normal because it's through the janitor.technology webapp?

Comment 21

[Jan Keromnes [:janx]]

(In reply to patrickkerry.pei from comment #20)
> I think I finished fixing all the errors

That's awesome! Thanks again for baring with us through the Windows setup.

> but have a question about the
> 'target' prop that originates from the client (DebuggerClient)'s call to
> listAddons(), which subsequently makes a call to the DebuggerClient's
> requester function...(this is all in ./devtools/shared/main.js) Is there
> somewhere I can find the documentation for the return object?

The requester function ends up relaying the call to the corresponding devtools actor in the server, and the response to such a call will always be a JSON object (no cyclical references or complex types, so that it can be stringified). The response usually has a `from` string attribute (the addonID of the actor that's replying to the call), but its exact shape varies according to the specific actor method being called, and if there was an error or not.

Looking at `RootActor.onListAddons`[0], we can see that the returned object will look like `{from:string, addons:array}` (or `{from:string, error:string, message:string}` if there was an error). More specifically, `addons` will be an Array of addonActor forms that look like [1], but we don't have to match this shape very precisely.

[0] https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/root.js#333
[1] https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/addon.js#81

> Also, I noticed that in comment 8, you listed the target actors as string
> props - is there a reason for that? I thought that
> https://developer.mozilla.org/en-US/Add-ons/SDK/Low-Level_APIs/dev_panel#Actors
> indicated that each Actor is a Vulcan.js object?

Interesting, I didn't know about Volcan.js.

What we call `workerActor` here is actually a bit of a misnomer, because it's just the actorID string of an actual `BrowserAddonActor` (so that it can be transmitted via the protocol). That string should always have a recognizable format, but it's OK to validate the prop just as a string.

> Lastly, does the process of submitting a patch differ at all from normal
> because it's through the janitor.technology webapp?

Submitting a patch from the Janitor works using the Bugzilla API, via the command `git bz attach`. This is temporary, as uploading a patch will soon be handled by a simple button click in the IDE, but for now, please run the following commands:

    git config --global core.editor vim # or your preferred command line editor
    git config --global bz.username <your_bugzilla_email>
    git config --global bz.apikey <your_bugzilla_api_key> # from https://bugzilla.mozilla.org/userprefs.cgi?tab=apikey
    git bz attach <git_commit_or_range>

Comment 22

[patrickkerry.pei]

Attachment: fix eslint react/prop-types rule

Comment 23

[Jan Keromnes [:janx]]

Comment on attachment 8776339 [details] [diff] [review]
fix eslint react/prop-types rule

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

Hi Patrick, thanks a lot for this patch! It looks great, and it already feels close to finished.

I added a few comments below about specific props, please address them and upload a new version of the patch when it's ready.

Note: The upstream code has changed somewhat, please be sure to run `git pull --rebase origin master` and fix any conflicts that appear.

Also, it's great that `./mach eslint devtools/aboutdebugging` works without errors, but please also try running about:debugging in Firefox and verify that the props really match their validation during runtime. The way I did this was:

    # replace `react.js` by `react-dev.js`:
    cp devtools/client/shared/vendor/react{-dev,}.js
    ./mach build devtools

    # then in Janitor's VNC interface, run:
    ./mach run about:debugging -jsconsole

This will open about:debugging in Firefox, and also the BrowserConsole in a separate window. When using about:debugging (e.g. switching from "Add-ons" category to "Tabs" and "Workers"), you should see some errors and warnings appear in the BrowserConsole. If you see errors about a prop validation problem, please try to fix it (you don't have to fix other errors like "key" problems etc.).

> # HG changeset patch
> # User Patrick <[e-mail]>
>  
> Bug 1264929 fix eslint react/prop-types rule

Please amend your commit message to "Bug 1264929 - Fix eslint react/prop-types rule. r=janx" (with dash, capital F, dot and r=janx).

::: devtools/client/aboutdebugging/components/aboutdebugging.js
@@ +6,5 @@
>  
>  "use strict";
>  
> +loader.lazyRequireGetter(this, "DebuggerClient",
> +  "devtools/shared/client/main", true);

Nit: Please move this `loader.lazyRequireGetter` below the `loader.lazyGetter` lines in this file.

@@ +53,5 @@
> +        return new Error(componentName + " expects prop `" + propName +
> +          "` to be an instance of `DebuggerClient`.");
> +      }
> +      return null;
> +    }),

Nit: `client` is not supposed to be an array of DebuggerClient objects, but actually a single DebuggerClient itself.

There is also a simpler way to specify this: `client: PropTypes.instanceOf(DebuggerClient)`. Please use this instead of the ad-hoc function with custom Error creation code.

Also, please add `.isRequired` at the end.

@@ +58,5 @@
> +    telemetry: PropTypes.shape({
> +      destroy: PropTypes.func.isRequired,
> +      toolClosed: PropTypes.func.isRequired,
> +      toolOpened: PropTypes.func.isRequired
> +    })

Nit: Please replace this `telemetry` shape with `PropTypes.instanceOf(Telemetry).isRequired`.

Here is how to require it:

    loader.lazyRequireGetter(this, "Telemetry",
      "devtools/client/shared/telemetry");

::: devtools/client/aboutdebugging/components/addons/panel.js
@@ +4,5 @@
>  
>  "use strict";
>  
> +loader.lazyRequireGetter(this, "DebuggerClient",
> +  "devtools/shared/client/main", true);

Nit: Please move this `loader.lazyRequireGetter` below the block of `createFactory` lines.

@@ +33,5 @@
> +        return new Error(componentName + " expects prop `" + propName +
> +          "` to be an instance of `DebuggerClient`.");
> +      }
> +      return null;
> +    }),

Nit: `client: PropTypes.instanceOf(DebuggerClient).isRequired`.

::: devtools/client/aboutdebugging/components/addons/target.js
@@ +7,5 @@
>  
>  "use strict";
>  
> +loader.lazyRequireGetter(this, "DebuggerClient",
> +  "devtools/shared/client/main", true);

Nit: Please move this `lazyRequireGetter` below the block of `require` lines (above the `Services.strings.createBundle` line).

@@ +29,5 @@
> +        return new Error(componentName + " expects prop `" + propName +
> +          "` to be an instance of `DebuggerClient`.");
> +      }
> +      return null;
> +    }),

Nit: `client: PropTypes.instanceOf(DebuggerClient).isRequired`.

@@ +32,5 @@
> +      return null;
> +    }),
> +    debugDisabled: PropTypes.bool,
> +    target: PropTypes.shape({
> +      addonActor: PropTypes.string,

Nit: Please add `.isRequired` here.

@@ +33,5 @@
> +    }),
> +    debugDisabled: PropTypes.bool,
> +    target: PropTypes.shape({
> +      addonActor: PropTypes.string,
> +      addonID: PropTypes.string,

Nit: Please add `.isRequired` here.

@@ +37,5 @@
> +      addonID: PropTypes.string,
> +      icon: PropTypes.string,
> +      name: PropTypes.string.isRequired,
> +      temporarilyInstalled: PropTypes.bool
> +    })

Nit: Please add `.isRequired` at the end of `target: PropTypes.shape({...})`.

::: devtools/client/aboutdebugging/components/panel-header.js
@@ +11,5 @@
>    displayName: "PanelHeader",
>  
> +  propTypes: {
> +    id: PropTypes.string,
> +    name: PropTypes.string

Nit: Please add `.isRequired` to both props.

::: devtools/client/aboutdebugging/components/panel-menu-entry.js
@@ +14,5 @@
> +    icon: PropTypes.string,
> +    id: PropTypes.string,
> +    name: PropTypes.string,
> +    selected: PropTypes.bool,
> +    selectPanel: PropTypes.func

Nit: Please add `.isRequired` to the props `id`, `name` and `selectPanel`.

::: devtools/client/aboutdebugging/components/panel-menu.js
@@ +15,5 @@
> +    panels: PropTypes.arrayOf(PropTypes.shape({
> +      id: PropTypes.string,
> +      name: PropTypes.string,
> +      icon: PropTypes.string,
> +      component: PropTypes.element

Nit: `component` is not a React element, but a React class/factory. Please use `PropTypes.func.isRequired` instead.

@@ +16,5 @@
> +      id: PropTypes.string,
> +      name: PropTypes.string,
> +      icon: PropTypes.string,
> +      component: PropTypes.element
> +    })),

Nit: Please add `.isRequired` after `panels: PropTypes.arrayOf(...)`, and after all 4 shape props.

@@ +17,5 @@
> +      name: PropTypes.string,
> +      icon: PropTypes.string,
> +      component: PropTypes.element
> +    })),
> +    selectPanel: PropTypes.func,

Nit: Please add `.isRequired` here.

::: devtools/client/aboutdebugging/components/tabs/panel.js
@@ +6,5 @@
>  
>  "use strict";
>  
> +loader.lazyRequireGetter(this, "DebuggerClient",
> +  "devtools/shared/client/main", true);

Nit: Please move this `lazyRequireGetter` below the block of `createFactory` lines.

@@ +29,5 @@
> +        return new Error(componentName + " expects prop `" + propName +
> +          "` to be an instance of `DebuggerClient`.");
> +      }
> +      return null;
> +    }),

Nit: `client: PropTypes.instanceOf(DebuggerClient).isRequired`.

@@ +30,5 @@
> +          "` to be an instance of `DebuggerClient`.");
> +      }
> +      return null;
> +    }),
> +    id: PropTypes.string

Nit: Tab IDs should match `PropTypes.number` instead. Also please add `.isRequired` here.

::: devtools/client/aboutdebugging/components/tabs/target.js
@@ +18,5 @@
>  
> +  propTypes: {
> +    target: PropTypes.shape({
> +      icon: PropTypes.string,
> +      outerWindowID: PropTypes.string,

Nit: `outerWindowID` should match `PropTypes.number` instead. Also `.isRequired` please.

@@ +20,5 @@
> +    target: PropTypes.shape({
> +      icon: PropTypes.string,
> +      outerWindowID: PropTypes.string,
> +      title: PropTypes.string,
> +      url: PropTypes.string

Nit: Please add `.isRequired` here.

@@ +21,5 @@
> +      icon: PropTypes.string,
> +      outerWindowID: PropTypes.string,
> +      title: PropTypes.string,
> +      url: PropTypes.string
> +    })

Nit: Please add `.isRequired` to `target`.

::: devtools/client/aboutdebugging/components/target-list.js
@@ +4,5 @@
>  
>  "use strict";
>  
> +loader.lazyRequireGetter(this, "DebuggerClient",
> +  "devtools/shared/client/main", true);

Nit: Please move this `lazyRequireGetter` below the block of `require` lines.

@@ +27,5 @@
> +        return new Error(componentName + " expects prop `" + propName +
> +          "` to be an instance of `DebuggerClient`.");
> +      }
> +      return null;
> +    }),

Nit: `client: PropTypes.instanceOf(DebuggerClient).isRequired`.

@@ +29,5 @@
> +      }
> +      return null;
> +    }),
> +    debugDisabled: PropTypes.bool,
> +    error: PropTypes.Error,

Nit: I don't think `PropTypes.Error` exists. Instead, please match `PropTypes.node`.

@@ +32,5 @@
> +    debugDisabled: PropTypes.bool,
> +    error: PropTypes.Error,
> +    id: PropTypes.string,
> +    name: PropTypes.string,
> +    sort: PropTypes.func,

Nit: This is actually a boolean, so `PropTypes.bool`.

@@ +33,5 @@
> +    error: PropTypes.Error,
> +    id: PropTypes.string,
> +    name: PropTypes.string,
> +    sort: PropTypes.func,
> +    targetClass: PropTypes.element,

Nit: This is a class/factory, please use `PropTypes.func.isRequired`.

@@ +34,5 @@
> +    id: PropTypes.string,
> +    name: PropTypes.string,
> +    sort: PropTypes.func,
> +    targetClass: PropTypes.element,
> +    targets: PropTypes.shape

Nit: This should be a `PropTypes.arrayOf(PropTypes.object).isRequired`. (No need to precisely specify target shape here, as this will be done in the `targetClass` anyway, so `PropTypes.object` is enough).

::: devtools/client/aboutdebugging/components/workers/panel.js
@@ +6,5 @@
>  
>  loader.lazyImporter(this, "PrivateBrowsingUtils",
>    "resource://gre/modules/PrivateBrowsingUtils.jsm");
> +loader.lazyRequireGetter(this, "DebuggerClient",
> +  "devtools/shared/client/main", true);

Nit: Please move both `lazyImporter` and `lazyRequireGetter` below the block of `require` calls.

@@ +35,5 @@
> +        return new Error(componentName + " expects prop `" + propName +
> +          "` to be an instance of `DebuggerClient`.");
> +      }
> +      return null;
> +    }),

Nit: `client: PropTypes.instanceOf(DebuggerClient).isRequired`.

@@ +36,5 @@
> +          "` to be an instance of `DebuggerClient`.");
> +      }
> +      return null;
> +    }),
> +    id: PropTypes.string

Nit: Please add `.isRequired` here.

::: devtools/client/aboutdebugging/components/workers/service-worker-target.js
@@ +6,5 @@
>  
>  "use strict";
>  
> +loader.lazyRequireGetter(this, "DebuggerClient",
> +  "devtools/shared/client/main", true);

Nit: Please move below.

@@ +26,5 @@
> +        return new Error(componentName + " expects prop `" + propName +
> +          "` to be an instance of `DebuggerClient`.");
> +      }
> +      return null;
> +    }),

Nit: `client: PropTypes.instanceOf(DebuggerClient).isRequired`.

@@ +35,5 @@
> +      url: PropTypes.string,
> +      scope: PropTypes.string,
> +      registrationActor: PropTypes.string.isRequired,
> +      workerActor: PropTypes.string
> +    })

Nit: Please add `.isRequired` to `target`.

::: devtools/client/aboutdebugging/components/workers/target.js
@@ +6,5 @@
>  
>  "use strict";
>  
> +loader.lazyRequireGetter(this, "DebuggerClient",
> +  "devtools/shared/client/main", true);

Nit: Please move below.

@@ +26,5 @@
> +        return new Error(componentName + " expects prop `" + propName +
> +          "` to be an instance of `DebuggerClient`.");
> +      }
> +      return null;
> +    }),

Nit: `client: PropTypes.instanceOf(DebuggerClient).isRequired`.

@@ +32,5 @@
> +    target: PropTypes.shape({
> +      icon: PropTypes.string,
> +      name: PropTypes.string.isRequired,
> +      workerActor: PropTypes.string
> +    })

Nit: Please add `.isRequired` to `target`.

Comment 24

[patrickkerry.pei]

Attachment: Fix eslint react/prop-types rule.

Comment 25

[patrickkerry.pei]

I've applied the changes but was unable to use The Janitor's VNC - I initially had a typo in a require statement which caused the connection to freeze and none of the options in the top right is able to reset the connection.

Is there anyway to manually forcefully disconnect? I'd really like to make sure there aren't any more errors in about:debugging!

Comment 26

[patrickkerry.pei]

(Setting the need info flag but absolutely no rush)

Comment 27

[Jan Keromnes [:janx]]

Comment on attachment 8776339 [details] [diff] [review]
fix eslint react/prop-types rule

Marking previous patch as obsolete.

Note to Patrick: You can do this directly from `git bz attach -e HEAD` by removing the "#" in front of the line that obsoletes an old attachment.

Comment 28

[Jan Keromnes [:janx]]

Comment on attachment 8778634 [details] [diff] [review]
Fix eslint react/prop-types rule.

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

Works great, thanks a lot Patrick!

I give this patch an "R+ with nits" (please fix the minor nits below, then re-upload a patch, but you can carry over my R+ to the new patch).

As usual, please don't forget to rebase your patch to the latest sources.

::: devtools/client/aboutdebugging/components/aboutdebugging.js
@@ +17,5 @@
>  loader.lazyGetter(this, "TabsPanel",
>    () => createFactory(require("./tabs/panel")));
>  loader.lazyGetter(this, "WorkersPanel",
>    () => createFactory(require("./workers/panel")));
> +loader.lazyRequireGetter(this, "DebuggerClient",

Nit: Please separate `lazyGetter` and `lazyRequireGetter` code blocks with an empty line.

::: devtools/client/aboutdebugging/components/addons/panel.js
@@ +28,5 @@
>    displayName: "AddonsPanel",
>  
> +  propTypes: {
> +    client: PropTypes.instanceOf(DebuggerClient).isRequired,
> +    id: PropTypes.string

Nit: Please declare `id` as required here.

::: devtools/client/aboutdebugging/components/addons/target.js
@@ +6,5 @@
>  
>  "use strict";
>  
> +loader.lazyImporter(this, "BrowserToolboxProcess",
> +  "resource://devtools/client/framework/ToolboxProcess.jsm");

Nit: Please move this `lazyImporter` block below, between the `require` block and the `lazyRequireGetter` block.

::: devtools/client/aboutdebugging/components/panel-menu-entry.js
@@ +10,5 @@
>  module.exports = createClass({
>    displayName: "PanelMenuEntry",
>  
> +  propTypes: {
> +    icon: PropTypes.string,

Nit: Actually, please make this icon required as well (we wouldn't want to accept new menu entries without an icon).

::: devtools/client/aboutdebugging/components/tabs/panel.js
@@ +24,5 @@
>    displayName: "TabsPanel",
>  
> +  propTypes: {
> +    client: PropTypes.instanceOf(DebuggerClient).isRequired,
> +    id: PropTypes.number.isRequired

Nit: Sorry, I told you that this is a `PropTypes.number`, but actually you were right in the previous patch, `id` is actually a `PropTypes.string`. (My mistake, I confused it with `outerWindowID`).

::: devtools/client/aboutdebugging/components/target-list.js
@@ +24,5 @@
> +  propTypes: {
> +    client: PropTypes.instanceOf(DebuggerClient).isRequired,
> +    debugDisabled: PropTypes.bool,
> +    error: PropTypes.node,
> +    id: PropTypes.string,

Nit: Please declare `id` as required here.

::: devtools/client/aboutdebugging/components/workers/panel.js
@@ +16,5 @@
>  const ServiceWorkerTarget = createFactory(require("./service-worker-target"));
>  
> +loader.lazyImporter(this, "PrivateBrowsingUtils",
> +  "resource://gre/modules/PrivateBrowsingUtils.jsm");
> +loader.lazyRequireGetter(this, "DebuggerClient",

Nit: Please separate the `lazyImporter` block from the `lazyRequireGetter` block with an empty line.

::: devtools/client/aboutdebugging/components/workers/service-worker-target.js
@@ +26,5 @@
> +    target: PropTypes.shape({
> +      icon: PropTypes.string,
> +      name: PropTypes.string.isRequired,
> +      url: PropTypes.string,
> +      scope: PropTypes.string,

Nit: Please make the scope required.

Comment 29

[Jan Keromnes [:janx]]

(In reply to patrickkerry.pei from comment #25)
> I've applied the changes but was unable to use The Janitor's VNC - I
> initially had a typo in a require statement which caused the connection to
> freeze and none of the options in the top right is able to reset the
> connection.
> 
> Is there anyway to manually forcefully disconnect? I'd really like to make
> sure there aren't any more errors in about:debugging!

Hm, that's unfortunate. Usually to forcefully re-connect, you just need to refresh the noVNC tab.

If that doesn't work, you could also try navigating to https://janitor.technology/vnc/firefox/0/vnc.html (the manual interface), but if there really is a typo breaking noVNC, it won't work either.

Maybe your Firefox environment on Janitor is based on a noVNC version that's broken (it sometimes happens when they publish a broken revision on their master branch). In that case, you can either:

1) Try to fix the noVNC but in /home/user/.novnc/, and force noVNC to restart by running `killall python && /home/user/.novnc/utils/launch.sh --vnc localhost:5900 --listen 8088 &`

2) Or simply save your work, destroy your Firefox workspace from https://janitor.technology/contributions/, then re-open a new one from https://janitor.technology/projects/

If that's really the problem your seeing, I'm very sorry about that! In the future, I'll make sure that Janitor project images are based on dependency revisions that really work.

Comment 30

[Patrick Brosset <:pbro>]

[good first bug] whiteboard -> keyword mass change

Comment 31

[patrickkerry.pei]

Attachment: Attachment to Bug 1264929 - Make about:debugging pass ESLint's "react/prop-types" rule.

Fix eslint react/prop-types rule.

Comment 32

[Mark Banner (:standard8)]

Jan, this looks ready to land, but possibly slipped through the cracks, can you take a look & help move it forward?

Comment 33

[Jan Keromnes [:janx]]

Attachment: Make about:debugging pass ESLint's "react/prop-types" rule.

Indeed, thank you Mark for catching this!

Patrick, I'm very sorry that I missed your last patch. In the future, please remember to use either needinfo or review flags when you expect a code review. Also, thanks again for all the great work you did on this bug! You persevered in spite of many road blocks, and we're very thankful for that.

I took the liberty of rebasing your patch, and cleaned it up a little (removed trailing spaces, removed the react dev/prod hack, and fixed two small new issues).

Julian, this patch adds prop validation to all about:debugging modules. You can test it by changing "react/prop-types" from 0 to 2 in /devtools/.eslintrc.js (you might actually want to land this change permanently, and add the few non-compliant devtools modules outside of about:debugging to /.eslintignore), and run `./mach eslint devtools/client/aboutdebugging/`.

You can also test the props validation at runtime, by replacing react.js by react-dev.js, and rebuild, as explained in comment 23. You'll then see validation errors in the JSConsole when manually testing about:debugging (there are currently no errors though, so you might have to create some on purpose to see them in the JSConsole).

I've reviewed previous versions of this patch, and had a quick look at this newer version, but it's probably better if you give it one final review(s). Thanks!

Comment 34

[Julian Descottes [:jdescottes]]

Comment on attachment 8802865 [details] [diff] [review]
Make about:debugging pass ESLint's "react/prop-types" rule.

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

Looks good to me, let's just wait for try before landing:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e909b954b9bf05e1b18095fab8e035f9cf64409c

Comment 35

[Julian Descottes [:jdescottes]]

Try is green, adding checkin needed!

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2631d596fc32
Make about:debugging pass ESLint's "react/prop-types" rule. r=jdescottes

Comment 37

[Iris Hsiao [:ihsiao]]

Sorry  had to back out for dt1 failures on OS X and Win7,like https://treeherder.mozilla.org/logviewer.html#?job_id=38842976&repo=mozilla-inbound
Not sure if I should ni patrick or janx for this bug, so just both :)

Comment 38

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2a11ef66441
Backed out changeset 2631d596fc32 for dt1 failures on OS X and Win7

Comment 39

[Julian Descottes [:jdescottes]]

I will take care of amending the patch, the registrationActor prop is indeed optional for serviceworker Target elements, props validation should reflect that.

This kind of runtime error would only pops up on e10s debug though, since we use react dev only in debug, and registrationActor is only missing in e10s.

Comment 40

[Julian Descottes [:jdescottes]]

Attachment: bug1264929.amended.patch

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=89a05730c635ac4456d3af4143f6e9bdb425cc3f

Comment 41

[Julian Descottes [:jdescottes]]

This one looks good except for unrelated intermittents. Adding checkin-needed.

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d93778ee9f58
Make about:debugging pass ESLint's "react/prop-types" rule. r=jdescottes

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d93778ee9f58