[React DevTools] Move DevTools integration into its own repo #12316

gaearon · 2017-02-09T20:28:31Z

The way React DevTools integration was set up in RN was not entirely supported by the React team, and we had to disable it in c3b25c9 so that we can move forward with enabling Fiber support in React Native.

Here, I am moving the DevTools client setup from RN repo to React DevTools repo so that we can keep it in sync with the rest of React DevTools. This is also a part of a larger effort to consolidate DevTools code (facebook/react-devtools#489). It allows us to remove the double injection of the hook, an lets us replace the eval hack with a regular dependency. The implementation lives here now.

This change re-enables Nuclide Inspector with React Native Stack reconciler and prepares it for compatibility with React Native Fiber reconciler in the future. We will release more updates of react-devtools-core to improve Fiber compatibility as we are polishing it.

I am removing the packager proxying because setupDevtools has not been using it (for a while).

This only affects DEV codepath and has no impact on production.

Test Plan

After also applying #12305, stable Nuclide shows React inspector again on iOS.

So does Nuclide master:

I haven't tested the Android simulator.

gaearon · 2017-02-09T20:30:44Z

cc @javache @yungsters @spicyj

sahrens

I trust that you tested this and that everything works well :)

sahrens · 2017-02-10T01:41:12Z

Libraries/Core/Devtools/setupDevtools.js

+      'localhost',
+    // Read the optional global variable for backward compatibility.
+    // It was added in https://github.com/facebook/react-native/commit/bf2b435322e89d0aeee8792b1c6e04656c2719a0.
+    port: window.__REACT_DEVTOOLS_PORT__,


Do we need the 8097 fallback or is that baked into react-devtools-core now?

It's here now.

sahrens · 2017-02-10T01:41:34Z

local-cli/server/runServer.js


      wsProxy = webSocketProxy.attachToServer(serverInstance, '/debugger-proxy');
      ms = messageSocket.attachToServer(serverInstance, '/message');
-      webSocketProxy.attachToServer(serverInstance, '/devtools');


What was this used for before? Are you sure nothing is using it any more?

Not that I'm aware of. It was used at some point in the past because we used to proxy DevTools connection through the packager port. However we stopped doing that in favor of a hardcoded different port anyway, so the proxy was not used since then. I verified that contacting / directly (instead of /devtools) works just fine so that's what I'm doing now.

sahrens · 2017-02-10T01:43:10Z

Did you test both internal fb setup/nuclide and an open source setup?

gaearon · 2017-02-10T20:14:15Z

Tested with Catalyst and (censored), uncovered a bug related to Relay integration (fixed in facebook/react-devtools#505), and an annoying UX problem (fixed in facebook/react-devtools#506). All should be fixed now.

I slightly tweaked how the file is imported to make it clear that code is only ever supposed to run once (it used to export a function called once but doing it during init it more explicit). Also put it also inside a DEV block just in case somebody removes the DEV condition on the import.

I think this is good now (definitely better than Inspector not working 😉 ) so gonna get it in.

gaearon · 2017-02-10T20:28:21Z

Tested against UIExplorer too.

facebook-github-bot · 2017-02-10T20:29:23Z

@gaearon has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2017-02-11T09:48:12Z

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

Summary: The way React DevTools integration was set up in RN was not entirely supported by the React team, and we had to disable it in c3b25c9 so that we can move forward with enabling Fiber support in React Native. Here, I am moving the DevTools client setup from RN repo to [React DevTools repo](https://github.com/facebook/react-devtools/blob/master/packages/react-devtools-core/README.md#requirereact-devtools-coreconnecttodevtoolsoptions) so that we can keep it in sync with the rest of React DevTools. This is also a part of a larger effort to consolidate DevTools code (facebook/react-devtools#489). It allows us to remove the double injection of the hook, an lets us replace the `eval` hack with a regular dependency. The implementation [lives here now](https://github.com/facebook/react-devtools/blob/master/packages/react-devtools-core/src/backend.js). This change re-enables Nuclide Inspector with React Native Stack reconciler and prepares it for compatibi Closes facebook#12316 Reviewed By: zertosh Differential Revision: D4545322 Pulled By: gaearon fbshipit-source-id: ab949916c1a92c6b41cd41e7e1edf9697a71de2e

Summary: The way React DevTools integration was set up in RN was not entirely supported by the React team, and we had to disable it in c3b25c9059f57ee8df5505628ff6221f11cf9234 so that we can move forward with enabling Fiber support in React Native. Here, I am moving the DevTools client setup from RN repo to [React DevTools repo](https://github.com/facebook/react-devtools/blob/master/packages/react-devtools-core/README.md#requirereact-devtools-coreconnecttodevtoolsoptions) so that we can keep it in sync with the rest of React DevTools. This is also a part of a larger effort to consolidate DevTools code (facebook/react-devtools#489). It allows us to remove the double injection of the hook, an lets us replace the `eval` hack with a regular dependency. The implementation [lives here now](https://github.com/facebook/react-devtools/blob/master/packages/react-devtools-core/src/backend.js). This change re-enables Nuclide Inspector with React Native Stack reconciler and prepares it for compatibi Closes facebook/react-native#12316 Reviewed By: zertosh Differential Revision: D4545322 Pulled By: gaearon fbshipit-source-id: ab949916c1a92c6b41cd41e7e1edf9697a71de2e

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. GH Review: review-needed labels Feb 9, 2017

gaearon mentioned this pull request Feb 9, 2017

Consolidate packages facebook/react-devtools#489

Closed

28 tasks

sahrens approved these changes Feb 10, 2017

View reviewed changes

[React DevTools] Move the implementation into React DevTools repo

0ad31ce

gaearon force-pushed the devtools branch from e5d69e0 to 0ad31ce Compare February 10, 2017 20:10

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Feb 10, 2017

facebook-github-bot added Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 11, 2017

facebook-github-bot closed this in 934cd82 Feb 14, 2017

gaearon deleted the devtools branch February 17, 2017 16:24

gaearon mentioned this pull request Feb 17, 2017

Waiting for roots to load... error message facebook/react-devtools#354

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[React DevTools] Move DevTools integration into its own repo #12316

[React DevTools] Move DevTools integration into its own repo #12316

Uh oh!

gaearon commented Feb 9, 2017 •

edited

Loading

Uh oh!

gaearon commented Feb 9, 2017

Uh oh!

sahrens left a comment

Uh oh!

sahrens Feb 10, 2017

Uh oh!

gaearon Feb 10, 2017

Uh oh!

sahrens Feb 10, 2017

Uh oh!

gaearon Feb 10, 2017

Uh oh!

sahrens commented Feb 10, 2017

Uh oh!

gaearon commented Feb 10, 2017

Uh oh!

gaearon commented Feb 10, 2017

Uh oh!

facebook-github-bot commented Feb 10, 2017

Uh oh!

facebook-github-bot commented Feb 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[React DevTools] Move DevTools integration into its own repo #12316

[React DevTools] Move DevTools integration into its own repo #12316

Uh oh!

Conversation

gaearon commented Feb 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Plan

Uh oh!

gaearon commented Feb 9, 2017

Uh oh!

sahrens left a comment

Choose a reason for hiding this comment

Uh oh!

sahrens Feb 10, 2017

Choose a reason for hiding this comment

Uh oh!

gaearon Feb 10, 2017

Choose a reason for hiding this comment

Uh oh!

sahrens Feb 10, 2017

Choose a reason for hiding this comment

Uh oh!

gaearon Feb 10, 2017

Choose a reason for hiding this comment

Uh oh!

sahrens commented Feb 10, 2017

Uh oh!

gaearon commented Feb 10, 2017

Uh oh!

gaearon commented Feb 10, 2017

Uh oh!

facebook-github-bot commented Feb 10, 2017

Uh oh!

facebook-github-bot commented Feb 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gaearon commented Feb 9, 2017 •

edited

Loading