Skip to content

Conversation

@huntie
Copy link
Member

@huntie huntie commented Jul 3, 2024

Summary

Note

Depends on #86, #87, and #88.

Actions comment feedback #87 (review) and fixes the originally highlighted bug with updating the window title when Welcome is not the first view loaded. The model observer that updates the window title is now mounted in the frontend entry point (rn_fusebox.ts), rather than within the Welcome panel.

All changes:

  • Relocate ReactNativeApplicationModel to front_end/core/sdk/.
    • Add public metadataCached member (used by Welcome panel).
  • Split single model observer:
    • Modify RNWelcomeImpl:
      • Remove window title logic.
      • Harden behaviour for reactNativeVersion — read cached value if received already (i.e. Welcome panel was opened later).
    • Add FuseboxReactNativeApplicationObserver at root entry point.

Test plan

Screen.Recording.2024-07-03.at.17.00.40.mov
  • Connect application (welcome panel as first screen)
    • ✅ Window title is set, React Native version label is populated
  • Navigate to Console panel
  • Kill and relaunch application
  • Reconnect DevTools
    • ✅ Window title is refreshed
    • ✅ React Native version label is populated upon navigating to Welcome panel

Upstreaming plan

  • This commit should be sent as a patch to the upstream devtools-frontend repo. I've reviewed the contribution guide.
  • This commit is React Native-specific and cannot be upstreamed.

}
}

new FuseboxReactNativeApplicationObserver(SDK.TargetManager.TargetManager.instance());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prior art for this pattern under front_end/entrypoints/: ExecutionContextSelector.

@huntie huntie force-pushed the fix-window-title-observer branch from a8268fc to 76d5235 Compare July 3, 2024 16:27
Copy link

@hoxyq hoxyq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you try moving model registration (e.g. SDKModel.register call) from core/sdk to rn_fusebox?

The source file for model can be moved to front_end/models/react_native, there is already model for React DevTools bindings.

Overall, looks good, awesome improvement!

@huntie
Copy link
Member Author

huntie commented Jul 4, 2024

@hoxyq Discussed offline. I think we can treat ReactNativeApplicationModel as part of our (forked) "core" models under sdk/, since it's a wrapper interface for a top level CDP domain. This isn't the end for our code organisation though!

@huntie huntie force-pushed the fix-window-title-observer branch from 76d5235 to 3b942ca Compare July 4, 2024 10:59
@huntie huntie force-pushed the fix-window-title-observer branch from 3b942ca to b9a3f2a Compare July 4, 2024 11:01
@huntie huntie merged commit 601b272 into facebook:main Jul 4, 2024
@huntie huntie deleted the fix-window-title-observer branch July 4, 2024 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants