1439673 - Fix React 16 warnings

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P2)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

When `ac_add_options --enable-debug-js-modules` is set in `.mozconfig` the browser console is polluted by React warnings.

Most of these warnings can be fixed by either:

- Set the correct propType because the wrong propType is set.
- Set a propType as optional.
- Change conditional event listeners from `condition && doSomething()` to `condition ? doSomething() : undefined`.
- Adding unique keys to Components anywhere we use `.map()`.

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Bug 1439673 - Fix React 16 warnings

I believe this fixes all warnings except for one. This warning appears the first time the memory panel is selected:
"Warning: Failed prop type: Calling PropTypes validators directly is not supported by the `prop-types` package. Use `PropTypes.checkPropTypes()` to call them. Read more at http://fb.me/use-check-prop-types
    in MemoryApp (created by Connect(MemoryApp))
    in Connect(MemoryApp) (created by bound createElementWithValidation)
    in bound createElementWithValidation
    in Provider"

This appears to be an issue with `devtools/client/memory/app.js` but I will log a new bug for this.

Review commit: https://reviewboard.mozilla.org/r/222766/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/222766/

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I have created bug 1440740 to address the remaining message.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8953555 [details]
Bug 1439673 - Fix React 16 warnings

https://reviewboard.mozilla.org/r/222766/#review228964

I have one question about removing the aria-role attribute in the memory app, and I think it'd be better if some changes have their own commit so we can have a bit more context about the issue and the fix, but overall this is looking good.
Thanks Mike

::: devtools/client/framework/components/toolbox-controller.js:54
(Diff revision 1)
> +  componentDidMount() {
> +    this.mounted = true;
> +  }
> +
>    shouldComponentUpdate() {
>      return this.state.canRender;
>    }
>  
>    componentWillUnmount() {
>      this.state.toolboxButtons.forEach(button => {
>        button.off("updatechecked", this.state.checkedButtonsUpdated);
>      });
> +    this.mounted = false;

this seems a bit more complex than a simple proptype warning. Could we move it to its own commit so we can have a proper description of what's goign on and what the fix does ?

::: devtools/client/memory/components/SnapshotListItem.js
(Diff revision 1)
>      }, L10N.getStr("snapshot.io.save"));
>  
>      let deleteButton = !snapshot.path ? void 0 : dom.div({
>        onClick: () => onDelete(snapshot),
>        className: "delete",
> -      "aria-role": "button",

why do we need this change ?

::: devtools/server/actors/utils/stack.js:41
(Diff revision 1)
> +    if (this._framesToIndices) {
> -    this._framesToIndices.clear();
> +      this._framesToIndices.clear();
> -    this._framesToIndices = null;
> +      this._framesToIndices = null;
> +    }
> +    if (this._framesToForms) {
> -    this._framesToForms.clear();
> +      this._framesToForms.clear();
> -    this._framesToForms = null;
> +      this._framesToForms = null;
> +    }

this does not seem related to React 16. Can we move it to its own commit ?

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8953555 [details]
Bug 1439673 - Fix React 16 warnings

https://reviewboard.mozilla.org/r/222766/#review228996

::: devtools/client/memory/components/SnapshotListItem.js
(Diff revision 1)
>      }, L10N.getStr("snapshot.io.save"));
>  
>      let deleteButton = !snapshot.path ? void 0 : dom.div({
>        onClick: () => onDelete(snapshot),
>        className: "delete",
> -      "aria-role": "button",

Because it throws the following warning:
`Warning: Invalid aria prop 'aria-role' on <div> tag. For details, see https://fb.me/invalid-aria-prop`

Looking at the specs `aria-role` is only valid on the following elements:

- button
- cell
- checkbox
- columnheader
- gridcell
- heading
- link
- menuitem
- menuitemcheckbox
- menuitemradio
- option
- radio
- row
- rowgroup
- rowheader
- switch
- tab
- tooltip
- tree
- treeitem

See:
https://www.w3.org/TR/wai-aria/#namefromcontent

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8953555 [details]
Bug 1439673 - Fix React 16 warnings

https://reviewboard.mozilla.org/r/222766/#review228996

> Because it throws the following warning:
> `Warning: Invalid aria prop 'aria-role' on <div> tag. For details, see https://fb.me/invalid-aria-prop`
> 
> Looking at the specs `aria-role` is only valid on the following elements:
> 
> - button
> - cell
> - checkbox
> - columnheader
> - gridcell
> - heading
> - link
> - menuitem
> - menuitemcheckbox
> - menuitemradio
> - option
> - radio
> - row
> - rowgroup
> - rowheader
> - switch
> - tab
> - tooltip
> - tree
> - treeitem
> 
> See:
> https://www.w3.org/TR/wai-aria/#namefromcontent

okay :) Could we file a bug to turn this div into a button instead ? Could make a nice good-first-bug

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8953555 [details]
Bug 1439673 - Fix React 16 warnings

https://reviewboard.mozilla.org/r/222766/#review228964

> this seems a bit more complex than a simple proptype warning. Could we move it to its own commit so we can have a proper description of what's goign on and what the fix does ?

Fixed in bug 1441113

> this does not seem related to React 16. Can we move it to its own commit ?

I can't remember the sequence that produced this so I have removed it and will fix it again once we know how to reproduce it.

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8953555 [details]
Bug 1439673 - Fix React 16 warnings

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/222766/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0f30b905da81
Fix React 16 warnings r=nchevobbe

Comment 10

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8953555 [details]
Bug 1439673 - Fix React 16 warnings

https://reviewboard.mozilla.org/r/222766/#review229106

::: devtools/client/inspector/fonts/fonts.js:41
(Diff revision 2)
>      this.onThemeChanged = this.onThemeChanged.bind(this);
>  
>      this.init();
>    }
>  
> +  componentWillMount() {

This change should probably be reverted since this file is not a React component.

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Gabriel [:gl] (ΦωΦ) from comment #10)
> Comment on attachment 8953555 [details]
> Bug 1439673 - Fix React 16 warnings
> 
> https://reviewboard.mozilla.org/r/222766/#review229106
> 
> ::: devtools/client/inspector/fonts/fonts.js:41
> (Diff revision 2)
> >      this.onThemeChanged = this.onThemeChanged.bind(this);
> >  
> >      this.init();
> >    }
> >  
> > +  componentWillMount() {
> 
> This change should probably be reverted since this file is not a React
> component.

lol... interesting that React reported that we can't use this.store.dispatch from the constructor.

I will log a follow-up to remove it... works fine if I remove the whole componentWillMount() method.

Comment 12

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/0f30b905da81