Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor model observers for window title / version label #90

Merged
merged 1 commit into from
Jul 4, 2024

Conversation

huntie
Copy link

@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
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.

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
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 facebookexperimental:main Jul 4, 2024
3 checks passed
@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