Closed Bug 1439673 Opened 4 years ago Closed 4 years ago

Fix React 16 warnings

Categories

(DevTools :: General, enhancement, P2)

enhancement

Tracking

(firefox60 fixed)

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: miker, Assigned: miker)

References

Details

Attachments

(1 file)

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()`.
Has Regression Range: --- → irrelevant
Has STR: --- → irrelevant
Duplicate of this bug: 1440585
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/
Attachment #8953555 - Flags: review?(nchevobbe)
I have created bug 1440740 to address the remaining message.
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 ?
Attachment #8953555 - Flags: review?(nchevobbe) → review+
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 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 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 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.
(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.
https://hg.mozilla.org/mozilla-central/rev/0f30b905da81
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.