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()`.

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/

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 ?

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.

Pushed by mratcliffe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0f30b905da81 Fix React 16 warnings r=nchevobbe

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.