1413860 - Update shared components to use prop-types and react-dom-factories

Status: RESOLVED FIXED

(DevTools :: Shared Components, enhancement, P2)

Description

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

There are a number of reasons for this:

1. We need to have the modules available for code from devtools-core, which uses them.
2. It begins the process of getting rid of React deprecation warnings.
3. It prepares us to upgrade to React 16.

Versioning info from devtools-core github repo:

"prop-types": "^15.6.0"
"react-dom-factories": "^1.0.2"

Comment 1

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

Attachment: Bug 1413860 - Shared components to use prop-types and react-dom-factories

@nchevobbe: Please wait for a green try before reviewing this.

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

Comment 2

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

Comment on attachment 8925912 [details]
Bug 1413860 - Shared components to use prop-types and react-dom-factories

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

Comment 3

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

Comment on attachment 8925912 [details]
Bug 1413860 - Shared components to use prop-types and react-dom-factories

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

Comment 4

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

The dt1 failures on windows 7 appear to be related to an earlier push so the try run should be classed as green.

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8925912 [details]
Bug 1413860 - Shared components to use prop-types and react-dom-factories

https://reviewboard.mozilla.org/r/197138/#review203244

Thanks Mike.
I have a few comments.
Also, I am wondering if we do need the production proptypes file.
Couln't we have a shim or something since it's useless in production (unless I'm wrong) ?
This would save some time to not load the whole prop-types file (it is 34KB)

::: devtools/client/shared/components/NotificationBox.js
(Diff revision 3)
>    renderNotification(notification) {
>      return (
>        div({
>          key: notification.value,
>          className: "notification",
> -        "data-key": notification.value,

this shouldn't be removed

::: devtools/client/shared/components/SidebarToggle.js:9
(Diff revision 3)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> -const { DOM, Component, PropTypes } = require("devtools/client/shared/vendor/react");
> +const { Component, PropTypes } = require("devtools/client/shared/vendor/react");

shouldn't we import propTypes separately too ?

::: devtools/client/shared/components/tabs/Tabs.js:54
(Diff revision 3)
> -          React.PropTypes.element
> +          PropTypes.element
>          ]).isRequired,
> -        showAllTabsMenu: React.PropTypes.bool,
> -        onAllTabsMenuClick: React.PropTypes.func,
> +        showAllTabsMenu: PropTypes.bool,
> +        onAllTabsMenuClick: PropTypes.func,
>  
> -        // Set true will only render selected panel on DOM. It's complete
> +        // Set true will only render selected panel on dom. It's complete

this shouldn't be changed :)

::: devtools/client/shared/vendor/REACT_PROP_TYPES_UPGRADING.md:2
(Diff revision 3)
> +[//]: # (
> +  This Source Code Form is subject to the terms of the Mozilla Public License, v. 2.0. If a copy of the MPL was not distributed with thisfile, You can obtain one at http://mozilla.org/MPL/2.0/.

s/thisfile/this file

Comment 6

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

Comment on attachment 8925912 [details]
Bug 1413860 - Shared components to use prop-types and react-dom-factories

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197138/diff/3-4/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9029ee56b1ad
Shared components to use prop-types and react-dom-factories r=nchevobbe

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/96de79fffd2c
Backed out changeset 9029ee56b1ad for mochitest clipboard failures on browser_jsterm_selfxss.js. r=backout on a CLOSED TREE

Comment 9

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 9029ee56b1ad (bug 1413860) for mochitest clipboard failures on browser_jsterm_selfxss.js 
https://hg.mozilla.org/integration/autoland/rev/9029ee56b1ad6efc4506c04b62c21e9f30c58a42

https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9029ee56b1ad6efc4506c04b62c21e9f30c58a42&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception

Comment 10

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

(In reply to Cristian Brindusan [:cristian_brindusan] from comment #9)
> Backed out changeset 9029ee56b1ad (bug 1413860) for mochitest clipboard
> failures on browser_jsterm_selfxss.js 
> https://hg.mozilla.org/integration/autoland/rev/
> 9029ee56b1ad6efc4506c04b62c21e9f30c58a42
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=autoland&revision=9029ee56b1ad6efc4506c04b62c21e9f30c58a42&filter-
> resultStatus=testfailed&filter-resultStatus=busted&filter-
> resultStatus=exception

We didn't have this on try but I will see if I can reproduce the problem locally.

I think bug 1410351 is a far more likely culprit.

Comment 11

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

Okay, 100% this patch that is causing the problem... looking into it.

Comment 12

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

Comment on attachment 8925912 [details]
Bug 1413860 - Shared components to use prop-types and react-dom-factories

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197138/diff/4-5/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c00cc232d689
Shared components to use prop-types and react-dom-factories r=nchevobbe

Comment 14

[Natalia Csoregi [:nataliaCs]]

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