1419828 - Remove devtools references to React.addons.TestUtils

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P2)

Comment 1

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

We did hope to remove the monkey patch to get this to work but we still need it in order for events to work properly inside XUL panels, which is a shame because it also seems to slow everything down considerable.

I have managed to get this working but I will most likely need to make the changes manually rather than use a codemod (we use a range of different require methods and we just can't account for them with a codemod).

Comment 2

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

We currently use React.addons.TestUtils from:

- devtools/client/netmonitor/test/require-helper.js
- devtools/client/responsive.html/test/browser/browser_device_custom_remove.js
- devtools/client/responsive.html/test/browser/browser_device_custom.js
- devtools/client/responsive.html/test/browser/head.js
- devtools/client/shared/components/test/mochitest/head.js
- devtools/client/shared/components/test/mochitest/test_tabs_accessibility.html
- devtools/client/shared/components/test/mochitest/test_tree_06.html
- devtools/client/shared/components/test/mochitest/test_tree_09.html
- devtools/client/shared/components/test/mochitest/test_tree_10.html
- devtools/client/shared/components/test/mochitest/test_tree_11.html
- devtools/client/webconsole/new-console-output/test/helpers.js
- devtools/client/webconsole/new-console-output/test/require-helper.js

Comment 3

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

Attachment: Bug 1419828 - Remove devtools references to React.addons.TestUtils

Changes:

- Added testUtils to React DOMs Proxy MonkeyPatch.
- Removed TestUtils where it is not needed.
- Added syntax identifiers to Markdown fences just because VSCode can do syntax highlighting in the markdown itself when you do that.
- In our require-helper.js files I have had to keep the name "react-addons-test-utils." This is because Enzyme uses the require paths to choose which adapters are needed (none are... yet)... we will need to use "react-addons-test-utils" instead of "react-dom/test-utils" as the path until we upgrade to React 16+.

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

Comment 4

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

Comment on attachment 8932157 [details]
Bug 1419828 - Remove devtools references to React.addons.TestUtils

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

Comment 5

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

Try is Green

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8932157 [details]
Bug 1419828 - Remove devtools references to React.addons.TestUtils

https://reviewboard.mozilla.org/r/203200/#review209278

This looks good to me, i only have a few minor comments.

Let's push if TRY is happy

::: devtools/client/netmonitor/test/require-helper.js:15
(Diff revision 2)
> +    // Enzyme uses the require paths to choose which adapters are needed... we need to use
> +    // react-addons-test-utils as the path until we upgrade to React 16+.

could we add a TODO and add the bug number blocking this ?

::: devtools/client/shared/components/test/mochitest/test_tree_10.html:24
(Diff revision 2)
>  <script type="application/javascript">
>  window.onload = Task.async(function* () {
>    try {
>      const ReactDOM = browserRequire("devtools/client/shared/vendor/react-dom");
> -    const React = browserRequire("devtools/client/shared/vendor/react");
> -    const { Simulate } = React.addons.TestUtils;
> +    const { createFactory } = browserRequire("devtools/client/shared/vendor/react");
> +    const Tree = createFactory(browserRequire("devtools/client/shared/components/Tree"));

weird this wasn't caught by eslint

::: devtools/client/shared/vendor/REACT_UPGRADING.md:233
(Diff revision 2)
> +        case "TestUtils":
> +          let ReactInternals = ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED;
> +          return lazyFunctionBinding(ReactInternals, "ReactTestUtils");

can we add a comment explaining what we do here ?

::: devtools/client/shared/vendor/react-dom-dev.js:5509
(Diff revision 2)
> +          case "TestUtils":
> +            let ReactInternals = ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED;
> +            return lazyFunctionBinding(ReactInternals, "ReactTestUtils");

ditto

::: devtools/client/shared/vendor/react-dom.js:5509
(Diff revision 2)
> +          case "TestUtils":
> +            let ReactInternals = ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED;
> +            return lazyFunctionBinding(ReactInternals, "ReactTestUtils");

ditto

Comment 7

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

Comment on attachment 8932157 [details]
Bug 1419828 - Remove devtools references to React.addons.TestUtils

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1460a53233c
Remove devtools references to React.addons.TestUtils r=nchevobbe

Comment 9

[Bogdan Tara[:bogdan_tara | bogdant]]

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