From e7f6210d5d417c5b6d4ba7f5cf96b40dbf70b9cd Mon Sep 17 00:00:00 2001 From: Emily Janzer Date: Wed, 23 Oct 2019 10:28:54 -0700 Subject: [PATCH] Don't attempt to connect to React devtools every 2s Summary: When testing out the NetworkOverlay, I noticed that we were creating a lot of WebSocket connections for localhost:8097. Rick found that this is because we're trying to connect to React devtools every 2 seconds: https://github.com/facebook/react/blob/master/packages/react-devtools-core/src/backend.js#L67 and it appears we create a new WebSocket every time. Dan suggested that we use opening the dev menu as a trigger for attempting to connect to React devtools. This diff uses RCTNativeAppEventEmitter to emit an event from native when the dev menu/dialog is shown, and listening to that event in JS to attempt to connect to devtools. I'm also making the change of passing in a websocket instead of just passing in the host + port; this way it will only attempt to connect once on each call to `connectToDevTools` (otherwise, we would attempt to reconnect every 2 seconds as soon as the dev menu is opened, and then the next time the menu is opened we'd so start that *again*, and so on - I could have it keep track of whether it's already connecting and avoid doing it again, but this is easier and should be sufficient, I think). We should probably also update the suggested troubleshooting tips on the devtools page to reflect this change, so that people don't get confused. Changelog: [General] [Fixed] Fix issue where we attempt to connect to React devtools every 2 seconds Reviewed By: mmmulani Differential Revision: D17919808 fbshipit-source-id: 4658d995c274574d22f2f54ea06d7f29ef2f54dc --- Libraries/Core/setUpDeveloperTools.js | 35 +---------- Libraries/Core/setUpReactDevTools.js | 60 +++++++++++++++++++ React/DevSupport/RCTDevMenu.m | 5 ++ .../devsupport/DevSupportManagerImpl.java | 4 ++ 4 files changed, 70 insertions(+), 34 deletions(-) create mode 100644 Libraries/Core/setUpReactDevTools.js diff --git a/Libraries/Core/setUpDeveloperTools.js b/Libraries/Core/setUpDeveloperTools.js index 9cc71d3ca8a..fe8ea0ac550 100644 --- a/Libraries/Core/setUpDeveloperTools.js +++ b/Libraries/Core/setUpDeveloperTools.js @@ -24,40 +24,7 @@ if (__DEV__) { // TODO (T45803484) Enable devtools for bridgeless RN if (!global.RN$Bridgeless) { if (!global.__RCTProfileIsProfiling) { - // not when debugging in chrome - // TODO(t12832058) This check is broken - if (!window.document) { - const AppState = require('../AppState/AppState'); - // $FlowFixMe Module is untyped - const reactDevTools = require('react-devtools-core'); - const getDevServer = require('./Devtools/getDevServer'); - - // Don't steal the DevTools from currently active app. - // Note: if you add any AppState subscriptions to this file, - // you will also need to guard against `AppState.isAvailable`, - // or the code will throw for bundles that don't have it. - const isAppActive = () => AppState.currentState !== 'background'; - - // Get hostname from development server (packager) - const devServer = getDevServer(); - const host = devServer.bundleLoadedFromServer - ? devServer.url.replace(/https?:\/\//, '').split(':')[0] - : 'localhost'; - - const viewConfig = require('../Components/View/ReactNativeViewViewConfig.js'); - - reactDevTools.connectToDevTools({ - isAppActive, - host, - // 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__, - resolveRNStyle: require('../StyleSheet/flattenStyle'), - nativeStyleEditorValidAttributes: Object.keys( - viewConfig.validAttributes.style, - ), - }); - } + require('./setUpReactDevTools'); // Set up inspector const JSInspector = require('../JSInspector/JSInspector'); diff --git a/Libraries/Core/setUpReactDevTools.js b/Libraries/Core/setUpReactDevTools.js new file mode 100644 index 00000000000..df9cdf95cb6 --- /dev/null +++ b/Libraries/Core/setUpReactDevTools.js @@ -0,0 +1,60 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + * @format + */ + +'use strict'; + +if (__DEV__) { + // $FlowFixMe Module is untyped + const reactDevTools = require('react-devtools-core'); + const connectToDevTools = () => { + // not when debugging in chrome + // TODO(t12832058) This check is broken + if (!window.document) { + const AppState = require('../AppState/AppState'); + const getDevServer = require('./Devtools/getDevServer'); + + // Don't steal the DevTools from currently active app. + // Note: if you add any AppState subscriptions to this file, + // you will also need to guard against `AppState.isAvailable`, + // or the code will throw for bundles that don't have it. + const isAppActive = () => AppState.currentState !== 'background'; + + // Get hostname from development server (packager) + const devServer = getDevServer(); + const host = devServer.bundleLoadedFromServer + ? devServer.url.replace(/https?:\/\//, '').split(':')[0] + : 'localhost'; + + // Read the optional global variable for backward compatibility. + // It was added in https://github.com/facebook/react-native/commit/bf2b435322e89d0aeee8792b1c6e04656c2719a0. + const port = + window.__REACT_DEVTOOLS_PORT__ != null + ? window.__REACT_DEVTOOLS_PORT__ + : 8097; + + const WebSocket = require('../WebSocket/WebSocket'); + const ws = new WebSocket('ws://' + host + ':' + port); + + const viewConfig = require('../Components/View/ReactNativeViewViewConfig.js'); + reactDevTools.connectToDevTools({ + isAppActive, + resolveRNStyle: require('../StyleSheet/flattenStyle'), + nativeStyleEditorValidAttributes: Object.keys( + viewConfig.validAttributes.style, + ), + websocket: ws, + }); + } + }; + + const RCTNativeAppEventEmitter = require('../EventEmitter/RCTNativeAppEventEmitter'); + RCTNativeAppEventEmitter.addListener('RCTDevMenuShown', connectToDevTools); + connectToDevTools(); // Try connecting once on load +} diff --git a/React/DevSupport/RCTDevMenu.m b/React/DevSupport/RCTDevMenu.m index 88e11e92ada..17dd7db4021 100644 --- a/React/DevSupport/RCTDevMenu.m +++ b/React/DevSupport/RCTDevMenu.m @@ -424,6 +424,11 @@ RCT_EXPORT_METHOD(show) _presentedItems = items; [RCTPresentedViewController() presentViewController:_actionSheet animated:YES completion:nil]; + + [_bridge enqueueJSCall:@"RCTNativeAppEventEmitter" + method:@"emit" + args:@[@"RCTDevMenuShown"] + completion:NULL]; } - (RCTDevMenuAlertActionHandler)alertActionHandlerForDevItem:(RCTDevMenuItem *__nullable)item diff --git a/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevSupportManagerImpl.java b/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevSupportManagerImpl.java index e1a9607f3c2..2e3f651fb16 100644 --- a/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevSupportManagerImpl.java +++ b/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevSupportManagerImpl.java @@ -47,6 +47,7 @@ import com.facebook.react.devsupport.interfaces.DevSupportManager; import com.facebook.react.devsupport.interfaces.ErrorCustomizer; import com.facebook.react.devsupport.interfaces.PackagerStatusCallback; import com.facebook.react.devsupport.interfaces.StackFrame; +import com.facebook.react.modules.core.RCTNativeAppEventEmitter; import com.facebook.react.modules.debug.interfaces.DeveloperSettings; import com.facebook.react.packagerconnection.RequestHandler; import com.facebook.react.packagerconnection.Responder; @@ -621,6 +622,9 @@ public class DevSupportManagerImpl }) .create(); mDevOptionsDialog.show(); + if (mCurrentContext != null) { + mCurrentContext.getJSModule(RCTNativeAppEventEmitter.class).emit("RCTDevMenuShown", null); + } } /** Starts of stops the sampling profiler */