From 28ded2c6cde7429b3e4add94c84bb10963bcd922 Mon Sep 17 00:00:00 2001 From: Alex Hunt Date: Wed, 12 Jun 2024 09:39:08 -0700 Subject: [PATCH] Move SessionMetadata to HostTargetDelegate (#44878) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/44878 A refactor moving `SessionMetadata` (now renamed as `HostTargetMetadata`) out of `inspectorTarget->connect()` calls into a `HostTargetDelegate::getMetadata` method. This provides a cleaner interface and location for extending metadata fields in future. Changelog: [Internal] Reviewed By: robhogan Differential Revision: D58288491 fbshipit-source-id: 67e8b9a3fb6d0b7966187fa98d9852222f242b9d --- packages/react-native/React/Base/RCTBridge.mm | 13 ++++++++----- .../jni/ReactInstanceManagerInspectorTarget.cpp | 14 ++++++++------ .../jni/ReactInstanceManagerInspectorTarget.h | 1 + .../runtime/jni/JReactHostInspectorTarget.cpp | 13 ++++++++----- .../runtime/jni/JReactHostInspectorTarget.h | 1 + .../ReactCommon/jsinspector-modern/HostAgent.cpp | 8 ++++---- .../ReactCommon/jsinspector-modern/HostAgent.h | 6 +++--- .../jsinspector-modern/HostTarget.cpp | 9 ++++----- .../ReactCommon/jsinspector-modern/HostTarget.h | 16 ++++++++++------ .../jsinspector-modern/tests/HostTargetTest.cpp | 4 +--- .../jsinspector-modern/tests/InspectorMocks.h | 3 +++ .../tests/JsiIntegrationTest.h | 8 +++++--- .../tests/ReactInstanceIntegrationTest.cpp | 7 ++----- .../runtime/platform/ios/ReactCommon/RCTHost.mm | 13 ++++++++----- 14 files changed, 66 insertions(+), 50 deletions(-) diff --git a/packages/react-native/React/Base/RCTBridge.mm b/packages/react-native/React/Base/RCTBridge.mm index b736e3d8784..be1b40fe146 100644 --- a/packages/react-native/React/Base/RCTBridge.mm +++ b/packages/react-native/React/Base/RCTBridge.mm @@ -197,6 +197,13 @@ class RCTBridgeHostTargetDelegate : public facebook::react::jsinspector_modern:: { } + facebook::react::jsinspector_modern::HostTargetMetadata getMetadata() override + { + return { + .integrationName = "iOS Bridge (RCTBridge)", + }; + } + void onReload(const PageReloadRequest &request) override { RCTAssertMainQueue(); @@ -458,11 +465,7 @@ RCT_NOT_IMPLEMENTED(-(instancetype)init) // This can happen if we're about to be dealloc'd. Reject the connection. return nullptr; } - return strongSelf->_inspectorTarget->connect( - std::move(remote), - { - .integrationName = "iOS Bridge (RCTBridge)", - }); + return strongSelf->_inspectorTarget->connect(std::move(remote)); }, {.nativePageReloads = true, .prefersFuseboxFrontend = true}); } diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/jni/ReactInstanceManagerInspectorTarget.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/jni/ReactInstanceManagerInspectorTarget.cpp index 323b0e0d426..87a8a4aa3d0 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/jni/ReactInstanceManagerInspectorTarget.cpp +++ b/packages/react-native/ReactAndroid/src/main/jni/react/jni/ReactInstanceManagerInspectorTarget.cpp @@ -56,12 +56,7 @@ ReactInstanceManagerInspectorTarget::ReactInstanceManagerInspectorTarget( [inspectorTarget = inspectorTarget_](std::unique_ptr remote) -> std::unique_ptr { - return inspectorTarget->connect( - std::move(remote), - { - .integrationName = - "Android Bridge (ReactInstanceManagerInspectorTarget)", - }); + return inspectorTarget->connect(std::move(remote)); }, {.nativePageReloads = true, .prefersFuseboxFrontend = true}); } @@ -103,6 +98,13 @@ void ReactInstanceManagerInspectorTarget::registerNatives() { }); } +jsinspector_modern::HostTargetMetadata +ReactInstanceManagerInspectorTarget::getMetadata() { + return { + .integrationName = "Android Bridge (ReactInstanceManagerInspectorTarget)", + }; +} + void ReactInstanceManagerInspectorTarget::onReload( const PageReloadRequest& /*request*/) { delegate_->onReload(); diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/jni/ReactInstanceManagerInspectorTarget.h b/packages/react-native/ReactAndroid/src/main/jni/react/jni/ReactInstanceManagerInspectorTarget.h index 4aead15d30e..7aa4721ef7b 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/jni/ReactInstanceManagerInspectorTarget.h +++ b/packages/react-native/ReactAndroid/src/main/jni/react/jni/ReactInstanceManagerInspectorTarget.h @@ -51,6 +51,7 @@ class ReactInstanceManagerInspectorTarget jsinspector_modern::HostTarget* getInspectorTarget(); // HostTargetDelegate methods + jsinspector_modern::HostTargetMetadata getMetadata() override; void onReload(const PageReloadRequest& request) override; void onSetPausedInDebuggerMessage( const OverlaySetPausedInDebuggerMessageRequest&) override; diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactHostInspectorTarget.cpp b/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactHostInspectorTarget.cpp index ff3c8c4bc83..d65677be351 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactHostInspectorTarget.cpp +++ b/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactHostInspectorTarget.cpp @@ -40,11 +40,7 @@ JReactHostInspectorTarget::JReactHostInspectorTarget( std::unique_ptr remote) -> std::unique_ptr { if (auto inspectorTarget = inspectorTargetWeak.lock()) { - return inspectorTarget->connect( - std::move(remote), - { - .integrationName = "Android Bridgeless (ReactHostImpl)", - }); + return inspectorTarget->connect(std::move(remote)); } // Reject the connection. return nullptr; @@ -86,6 +82,13 @@ void JReactHostInspectorTarget::registerNatives() { }); } +jsinspector_modern::HostTargetMetadata +JReactHostInspectorTarget::getMetadata() { + return { + .integrationName = "Android Bridgeless (ReactHostImpl)", + }; +} + void JReactHostInspectorTarget::onReload(const PageReloadRequest& request) { javaReactHostImpl_->reload("CDP Page.reload"); } diff --git a/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactHostInspectorTarget.h b/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactHostInspectorTarget.h index e27655793ae..aa6a250295f 100644 --- a/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactHostInspectorTarget.h +++ b/packages/react-native/ReactAndroid/src/main/jni/react/runtime/jni/JReactHostInspectorTarget.h @@ -58,6 +58,7 @@ class JReactHostInspectorTarget jsinspector_modern::HostTarget* getInspectorTarget(); // HostTargetDelegate methods + jsinspector_modern::HostTargetMetadata getMetadata() override; void onReload(const PageReloadRequest& request) override; void onSetPausedInDebuggerMessage( const OverlaySetPausedInDebuggerMessageRequest&) override; diff --git a/packages/react-native/ReactCommon/jsinspector-modern/HostAgent.cpp b/packages/react-native/ReactCommon/jsinspector-modern/HostAgent.cpp index f818438db6e..36efb78e951 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/HostAgent.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/HostAgent.cpp @@ -27,11 +27,11 @@ namespace facebook::react::jsinspector_modern { HostAgent::HostAgent( FrontendChannel frontendChannel, HostTargetController& targetController, - HostTarget::SessionMetadata sessionMetadata, + HostTargetMetadata hostMetadata, SessionState& sessionState) : frontendChannel_(frontendChannel), targetController_(targetController), - sessionMetadata_(std::move(sessionMetadata)), + hostMetadata_(std::move(hostMetadata)), sessionState_(sessionState) {} void HostAgent::handleRequest(const cdp::PreparsedRequest& req) { @@ -49,10 +49,10 @@ void HostAgent::handleRequest(const cdp::PreparsedRequest& req) { } // Send a log entry with the integration name. - if (sessionMetadata_.integrationName) { + if (hostMetadata_.integrationName) { sendInfoLogEntry( ANSI_COLOR_BG_YELLOW "Debugger integration: " + - *sessionMetadata_.integrationName); + *hostMetadata_.integrationName); } shouldSendOKResponse = true; diff --git a/packages/react-native/ReactCommon/jsinspector-modern/HostAgent.h b/packages/react-native/ReactCommon/jsinspector-modern/HostAgent.h index 3424f79bd9b..903a4be9e0f 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/HostAgent.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/HostAgent.h @@ -37,13 +37,13 @@ class HostAgent final { * \param targetController An interface to the HostTarget that this agent is * attached to. The caller is responsible for ensuring that the * HostTargetDelegate and underlying HostTarget both outlive the agent. - * \param sessionMetadata Metadata about the session that created this agent. + * \param hostMetadata Metadata about the host that created this agent. * \param sessionState The state of the session that created this agent. */ HostAgent( FrontendChannel frontendChannel, HostTargetController& targetController, - HostTarget::SessionMetadata sessionMetadata, + HostTargetMetadata hostMetadata, SessionState& sessionState); HostAgent(const HostAgent&) = delete; @@ -94,7 +94,7 @@ class HostAgent final { FrontendChannel frontendChannel_; HostTargetController& targetController_; - const HostTarget::SessionMetadata sessionMetadata_; + const HostTargetMetadata hostMetadata_; std::shared_ptr instanceAgent_; FuseboxClientType fuseboxClientType_{FuseboxClientType::Unknown}; bool isPausedInDebuggerOverlayVisible_{false}; diff --git a/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.cpp b/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.cpp index 2e682460a94..69cbf598a13 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.cpp @@ -29,7 +29,7 @@ class HostTargetSession { explicit HostTargetSession( std::unique_ptr remote, HostTargetController& targetController, - HostTarget::SessionMetadata sessionMetadata) + HostTargetMetadata hostMetadata) : remote_(std::make_shared(std::move(remote))), frontendChannel_( [remoteWeak = std::weak_ptr(remote_)](std::string_view message) { @@ -40,7 +40,7 @@ class HostTargetSession { hostAgent_( frontendChannel_, targetController, - std::move(sessionMetadata), + std::move(hostMetadata), state_) {} /** @@ -146,10 +146,9 @@ HostTarget::HostTarget(HostTargetDelegate& delegate) executionContextManager_{std::make_shared()} {} std::unique_ptr HostTarget::connect( - std::unique_ptr connectionToFrontend, - SessionMetadata sessionMetadata) { + std::unique_ptr connectionToFrontend) { auto session = std::make_shared( - std::move(connectionToFrontend), controller_, std::move(sessionMetadata)); + std::move(connectionToFrontend), controller_, delegate_.getMetadata()); session->setCurrentInstance(currentInstance_.get()); sessions_.insert(std::weak_ptr(session)); return std::make_unique( diff --git a/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.h b/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.h index 6e4fafbd30d..dc271e2e056 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/HostTarget.h @@ -36,6 +36,10 @@ class HostAgent; class HostCommandSender; class HostTarget; +struct HostTargetMetadata { + std::optional integrationName; +}; + /** * Receives events from a HostTarget. This is a shared interface that each * React Native platform needs to implement in order to integrate with the @@ -85,6 +89,11 @@ class HostTargetDelegate { virtual ~HostTargetDelegate(); + /** + * Returns a metadata object describing the host. + */ + virtual HostTargetMetadata getMetadata() = 0; + /** * Called when the debugger requests a reload of the page. This is called on * the thread on which messages are dispatched to the session (that is, where @@ -150,10 +159,6 @@ class HostTargetController final { class JSINSPECTOR_EXPORT HostTarget : public EnableExecutorFromThis { public: - struct SessionMetadata { - std::optional integrationName; - }; - /** * Constructs a new HostTarget. * \param delegate The HostTargetDelegate that will @@ -185,8 +190,7 @@ class JSINSPECTOR_EXPORT HostTarget * destructor execute. */ std::unique_ptr connect( - std::unique_ptr connectionToFrontend, - SessionMetadata sessionMetadata = {}); + std::unique_ptr connectionToFrontend); /** * Registers an instance with this HostTarget. diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tests/HostTargetTest.cpp b/packages/react-native/ReactCommon/jsinspector-modern/tests/HostTargetTest.cpp index d9a38c120d6..869c9f1de25 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/tests/HostTargetTest.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/tests/HostTargetTest.cpp @@ -49,9 +49,7 @@ class HostTargetTest : public Test { std::pair, MockRemoteConnection&> makeConnection() { size_t connectionIndex = remoteConnections_.objectsVended(); - auto toPage = page_->connect( - remoteConnections_.make_unique(), - {.integrationName = "HostTargetTest"}); + auto toPage = page_->connect(remoteConnections_.make_unique()); // We'll always get an onDisconnect call when we tear // down the test. Expect it in order to satisfy the strict mock. diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tests/InspectorMocks.h b/packages/react-native/ReactCommon/jsinspector-modern/tests/InspectorMocks.h index abc1acfe0a3..83c73de6a4a 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/tests/InspectorMocks.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/tests/InspectorMocks.h @@ -118,6 +118,9 @@ class MockInspectorPackagerConnectionDelegate class MockHostTargetDelegate : public HostTargetDelegate { public: // HostTargetDelegate methods + HostTargetMetadata getMetadata() override { + return {.integrationName = "MockHostTargetDelegate"}; + } MOCK_METHOD(void, onReload, (const PageReloadRequest& request), (override)); MOCK_METHOD( void, diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tests/JsiIntegrationTest.h b/packages/react-native/ReactCommon/jsinspector-modern/tests/JsiIntegrationTest.h index c7756139821..10132b6425d 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/tests/JsiIntegrationTest.h +++ b/packages/react-native/ReactCommon/jsinspector-modern/tests/JsiIntegrationTest.h @@ -91,9 +91,7 @@ class JsiIntegrationPortableTest : public ::testing::Test, void connect() { ASSERT_FALSE(toPage_) << "Can only connect once in a JSI integration test."; - toPage_ = page_->connect( - remoteConnections_.make_unique(), - {.integrationName = "JsiIntegrationTest"}); + toPage_ = page_->connect(remoteConnections_.make_unique()); using namespace ::testing; // Default to ignoring console messages originating inside the backend. @@ -179,6 +177,10 @@ class JsiIntegrationPortableTest : public ::testing::Test, private: // HostTargetDelegate methods + HostTargetMetadata getMetadata() override { + return {.integrationName = "JsiIntegrationTest"}; + } + void onReload(const PageReloadRequest& request) override { (void)request; reload(); diff --git a/packages/react-native/ReactCommon/jsinspector-modern/tests/ReactInstanceIntegrationTest.cpp b/packages/react-native/ReactCommon/jsinspector-modern/tests/ReactInstanceIntegrationTest.cpp index 5c32d0eaa89..0173466df99 100644 --- a/packages/react-native/ReactCommon/jsinspector-modern/tests/ReactInstanceIntegrationTest.cpp +++ b/packages/react-native/ReactCommon/jsinspector-modern/tests/ReactInstanceIntegrationTest.cpp @@ -94,11 +94,8 @@ void ReactInstanceIntegrationTest::SetUp() { "mock-vm", [hostTargetIfModernCDP](std::unique_ptr remote) -> std::unique_ptr { - auto localConnection = hostTargetIfModernCDP->connect( - std::move(remote), - { - .integrationName = "ReactInstanceIntegrationTest", - }); + auto localConnection = + hostTargetIfModernCDP->connect(std::move(remote)); return localConnection; }, // TODO: Allow customisation of InspectorTargetCapabilities diff --git a/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTHost.mm b/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTHost.mm index 893f0d4dd0d..8e0b105f2c5 100644 --- a/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTHost.mm +++ b/packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTHost.mm @@ -40,6 +40,13 @@ class RCTHostHostTargetDelegate : public facebook::react::jsinspector_modern::Ho { } + jsinspector_modern::HostTargetMetadata getMetadata() override + { + return { + .integrationName = "iOS Bridgeless (RCTHost)", + }; + } + void onReload(const PageReloadRequest &request) override { RCTAssertMainQueue(); @@ -233,11 +240,7 @@ class RCTHostHostTargetDelegate : public facebook::react::jsinspector_modern::Ho // This can happen if we're about to be dealloc'd. Reject the connection. return nullptr; } - return strongSelf->_inspectorTarget->connect( - std::move(remote), - { - .integrationName = "iOS Bridgeless (RCTHost)", - }); + return strongSelf->_inspectorTarget->connect(std::move(remote)); }, {.nativePageReloads = true, .prefersFuseboxFrontend = true}); }