mirror of
https://github.com/facebook/react-native.git
synced 2025-11-01 09:14:26 +00:00
Edmond Chui
9b977def6c
Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/48895 Changelog: [General][Breaking][Fixed] - removed a long-running loop causing the app to lag while attempting a connection to Metro Round 2: Sorry I broke VR/Java apps in D68023397. Helpful teammates have reverted and Jedi landed it via D68522537. This diff adds the missing method call that caused the crash: ``` makeNativeMethod( "didOpen", JCxxInspectorPackagerConnectionWebSocketDelegate::didOpen ) ``` Test plan has been updated to include testing VR Store. This error wasn't caught by existing automated tests, because it only impacts development builds while using Metro. vzaidman is leading the effort to bring Jest E2E tests on React Native DevTools, which could catch crashes like this. Original summary in D68023397: D65952134 fixed the auto-reconnection between Metro and the device. There's an existing "constructed = connected" contract as [discussed](https://www.internalfb.com/diff/D65952134?dst_version_fbid=3741052436109227&transaction_fbid=581445277659906): https://www.internalfb.com/code/fbsource/[1592525fbcbb]/xplat/js/react-native-github/packages/react-native/ReactCommon/jsinspector-modern/WebSocketInterfaces.h?lines=16-20 In compliance, busy-waiting was [introduced](https://www.internalfb.com/diff/D65952134?dst_version_fbid=896147259315683&transaction_fbid=427393513742494) in V4 to wait for the connection result in the constructor. xArthasx [discovered](https://www.internalfb.com/diff/D65952134?dst_version_fbid=896147259315683&transaction_fbid=1406890420289706) a performance issue from this impl via a profiling result. In favour of async connection results, we're going back to V3 design with the imperative `isConnected()` check to the interface. xArthasx has confirmed this fixes the perf issue. While I haven't found a compelling reason against removing this contract from the initial design in D52134592, please let me know if I've missed one. This also means there was a scenario where messages were sent before the websocket is open. Those were dropped silently previously (before the busy-waiting while loop was introduced): https://www.internalfb.com/code/fbsource/[f7113e167ee1]/fbobjc/VendorLib/SocketRocket/src/SocketRocket/SRWebSocket.m?lines=630-637 This means message senders must now consider the connection state, e.g. by maintaining a pre-connection message queue, if they need to guarantee the messages to be sent. Reviewed By: robhogan Differential Revision: D68559198 fbshipit-source-id: afb3d8bfbf949c3324cbf126e9f3e3fc25e50541