Debugger: Make /json/list ordered (facebook#45069)

Summary:
Pull Request resolved: facebook#45069

Currently, `/json/list` returns pages within each device in the iteration order of a C++ `unordered_map`, which doesn't tell us anything useful. Page IDs happen to be sequential, but only as an implementation detail.

Change this contract so that we guarantee ordering reflects addition order, allowing clients to consistently select e.g. most recently added page for a given device.

The implementation of this is as simple as switching from an `unordered_map` to a key-ordered`map`, because we already assign keys (page IDs) with an incrementing integer. Within the inspector proxy, devices already use an insertion (connection)-ordered JS `Map`, so we just document this guarantee.

Changelog:
[General][Changed] Debugger: Make `/json/list` return connection-addition-ordered targets.

Reviewed By: huntie

Differential Revision: D58735947

fbshipit-source-id: 7a132cc5e750475792a2b845afc9a42424690bf1

Author: robhogan

packages/dev-middleware/src/inspector-proxy/Device.js

@@ -491,6 +491,7 @@ export default class Device {
   // locations).
   async #handleMessageFromDevice(message: MessageFromDevice) {
     if (message.event === 'getPages') {
+      // Preserve ordering - getPages guarantees addition order.
       this.#pages = new Map(
         message.payload.map(({capabilities, ...page}) => [
           page.id,

packages/dev-middleware/src/inspector-proxy/InspectorProxy.js

@@ -43,6 +43,10 @@ const DEBUGGER_HEARTBEAT_INTERVAL_MS = 10000;
 const INTERNAL_ERROR_CODE = 1011;
 
 export interface InspectorProxyQueries {
+  /**
+   * Returns list of page descriptions ordered by device connection order, then
+   * page addition order.
+   */
   getPageDescriptions(): Array<PageDescription>;
 }
 

packages/react-native/ReactCommon/jsinspector-modern/InspectorInterfaces.cpp

@@ -73,7 +73,7 @@ class InspectorImpl : public IInspector {
   };
   mutable std::mutex mutex_;
   int nextPageId_{1};
-  std::unordered_map<int, Page> pages_;
+  std::map<int, Page> pages_;
   std::list<std::weak_ptr<IPageStatusListener>> listeners_;
 };
 
@@ -109,6 +109,8 @@ int InspectorImpl::addPage(
     InspectorTargetCapabilities capabilities) {
   std::scoped_lock lock(mutex_);
 
+  // Note: getPages guarantees insertion/addition order. As an implementation
+  // detail, incrementing page IDs takes advantage of std::map's key ordering.
   int pageId = nextPageId_++;
   assert(pages_.count(pageId) == 0 && "Unexpected duplicate page ID");
   pages_.emplace(
@@ -133,6 +135,8 @@ std::vector<InspectorPageDescription> InspectorImpl::getPages() const {
   std::scoped_lock lock(mutex_);
 
   std::vector<InspectorPageDescription> inspectorPages;
+  // pages_ is a std::map keyed on an incremental id, so this is insertion
+  // ordered.
   for (auto& it : pages_) {
     inspectorPages.push_back(InspectorPageDescription(it.second));
   }

packages/react-native/ReactCommon/jsinspector-modern/InspectorInterfaces.h

@@ -109,7 +109,11 @@ class JSINSPECTOR_EXPORT IInspector : public IDestructible {
   /// debuggable pages.
   virtual void removePage(int pageId) = 0;
 
-  /// getPages is called by the client to list all debuggable pages.
+  /**
+   * Called by the client to retrieve all debuggable pages.
+   * \returns A vector of page descriptions in the order in which they were
+   * added with \c addPage.
+   */
   virtual std::vector<InspectorPageDescription> getPages() const = 0;
 
   /**

packages/react-native/ReactCommon/jsinspector-modern/tests/InspectorPackagerConnectionTest.cpp

@@ -199,34 +199,51 @@ TEST_F(InspectorPackagerConnectionTest, TestGetPages) {
       "event": "getPages"
     })");
 
-  auto pageId = getInspectorInstance().addPage(
-      "mock-title",
+  auto pageId1 = getInspectorInstance().addPage(
+      "mock-title-1",
+      "mock-vm",
+      localConnections_
+          .lazily_make_unique<std::unique_ptr<IRemoteConnection>>(),
+      {.nativePageReloads = true});
+
+  auto pageId2 = getInspectorInstance().addPage(
+      "mock-title-2",
       "mock-vm",
       localConnections_
           .lazily_make_unique<std::unique_ptr<IRemoteConnection>>(),
       {.nativePageReloads = true});
 
-  // getPages now reports the page we registered.
+  // getPages now reports the page we registered, in the order added
   EXPECT_CALL(
       *webSockets_[0],
       send(JsonParsed(AllOf(
           AtJsonPtr("/event", Eq("getPages")),
           AtJsonPtr(
               "/payload",
-              ElementsAreArray({AllOf(
-                  AtJsonPtr("/app", Eq("my-app")),
-                  AtJsonPtr("/title", Eq("mock-title [C++ connection]")),
-                  AtJsonPtr("/id", Eq(std::to_string(pageId))),
-                  AtJsonPtr("/capabilities/nativePageReloads", Eq(true)),
-                  AtJsonPtr(
-                      "/capabilities/nativeSourceCodeFetching",
-                      Eq(false)))}))))))
+              ElementsAreArray(
+                  {AllOf(
+                       AtJsonPtr("/app", Eq("my-app")),
+                       AtJsonPtr("/title", Eq("mock-title-1 [C++ connection]")),
+                       AtJsonPtr("/id", Eq(std::to_string(pageId1))),
+                       AtJsonPtr("/capabilities/nativePageReloads", Eq(true)),
+                       AtJsonPtr(
+                           "/capabilities/nativeSourceCodeFetching",
+                           Eq(false))),
+                   AllOf(
+                       AtJsonPtr("/app", Eq("my-app")),
+                       AtJsonPtr("/title", Eq("mock-title-2 [C++ connection]")),
+                       AtJsonPtr("/id", Eq(std::to_string(pageId2))),
+                       AtJsonPtr("/capabilities/nativePageReloads", Eq(true)),
+                       AtJsonPtr(
+                           "/capabilities/nativeSourceCodeFetching",
+                           Eq(false)))}))))))
       .RetiresOnSaturation();
   webSockets_[0]->getDelegate().didReceiveMessage(R"({
       "event": "getPages"
     })");
 
-  getInspectorInstance().removePage(pageId);
+  getInspectorInstance().removePage(pageId1);
+  getInspectorInstance().removePage(pageId2);
 
   // getPages is back to reporting no pages.
   EXPECT_CALL(