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

Grab RCTPerfMonitor's devSettings from the bridge #1426

Merged

Conversation

amgleitman
Copy link
Member

Please select one of the following

  • I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
  • I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
  • I am making a fix / change for the macOS implementation of react-native
  • I am making a change required for Microsoft usage of react-native

Summary

We've seen an issue on iOS when doing remote web debugging while using integrated RN with a custom RCTDevSettingsDataSource that doesn't persist globally (unlike the default which uses NSUserSettings), a scenario useful when simultaneously running multiple RN instances with different dev settings.

When we reload the bridge upon establishing a remote debugging connection, we reinitialize all our modules. On iOS this includes PerfMonitor, which depends on DevSettings, which attempts to load the current values using the default NSUserSettings-based dev settings when it gets reinitialized. However, since the original custom data source is designed to keep dev settings compartmentalized to particular RN instances, our "global defaults" that we load upon initialization have a different value. This causes the bridge to reload once again, resulting in an infinite loop.

The fix here is to use the bridge's already established devSettings so we ensure we're using our local dev settings instead of trying to default to global ones.

Changelog

[iOS] [Fixed] - Fix issue with custom non-persistent RCTDevSettingsDataSource

Test Plan

Validated that this fixes the original issue while keeping remote debugging in RNTester okay. Initial tests show that for RCTPerfMonitor, [self->_moduleRegistry moduleForName:"DevSettings"] and [[self bridge] devSettings] return the same value, so this shouldn't affect the remote debugging process in other cases.

@amgleitman amgleitman requested a review from a team as a code owner September 8, 2022 21:19
@Saadnajmi
Copy link
Collaborator

Corollary: Could RCTPerfMonitor be brought to macOS? It sounds useful :)

@amgleitman
Copy link
Member Author

Corollary: Could RCTPerfMonitor be brought to macOS? It sounds useful :)

Possibly, although it might be worth thinking about whether or not it might be redundant.

RCTPerfMonitor's purpose is to display statistics like memory usage, heap usage, FPS, etc. There might be other tools on macOS that can do these things, although I don't know for certain. Either way, it could be something to look into at a later date.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants