react-native icon indicating copy to clipboard operation
react-native copied to clipboard

[Android][React DevTools] Support persisted settings in Android

Open rbalicki2 opened this issue 3 years ago • 6 comments

Summary

  • The SettingsManager turbomodule exists in iOS, but not for Android. Add support for it.
  • One accesses the SettingsManager TM via Libraries/Settings/Settings. Implement part of that for Android, leaving watchKeys and clearWatch as no-ops.

Turbomodule implementation

  • The Java implementation writes to/from a file in the application's directory. Clearing the storage associated with the app removes this file.
  • This file stores a JSON object that represents settings the user has set. In particular, it will store something like {"ReactDevTools::Settings::ConsolePatchSettings":"{\"consolePatchSettings\":{\"appendComponentStack\":false,\"breakOnConsoleErrors\":false,\"showInlineWarningsAndErrors\":true,\"hideConsoleLogsInStrictMode\":true,\"browserTheme\":\"light\"}}"} when used with React DevTools.

Integation with DevTools

  • This implementation works as-is with the DevTools implementation here (except per @motiz88 's comments, I will rename some params in both diffs.)

Changelog

[General] [Added] - Add a SettingsManager TurboModule to Android

Test Plan

  • Extensive manual testing

rbalicki2 avatar Oct 13 '22 05:10 rbalicki2

@rbalicki2 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot avatar Oct 13 '22 05:10 facebook-github-bot

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,747,828 -31,160
android hermes armeabi-v7a 7,151,023 -31,637
android hermes x86 8,059,869 -33,795
android hermes x86_64 8,030,590 -34,614
android jsc arm64-v8a 9,609,147 -28,338
android jsc armeabi-v7a 8,374,263 -28,817
android jsc x86 9,556,450 -30,996
android jsc x86_64 10,149,202 -31,800

Base commit: 31f219977ddf87c19b4c65dd74058fd3f081ef0a Branch: main

analysis-bot avatar Oct 13 '22 05:10 analysis-bot

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 31f219977ddf87c19b4c65dd74058fd3f081ef0a Branch: main

analysis-bot avatar Oct 13 '22 06:10 analysis-bot

I'm not sure this is a new public API we should be exposing. Settings has some specifically documented semantics for iOS (https://reactnative.dev/docs/settings), but the Android implementation here seems sort of like a more minimal version of AsyncStorage, which we have been working to move out of RN core itself (cc @rshest). Adding a new public API surface for persistent key/value storage seems to kind of conflict with that goal.

If React DevTools does need to use a different key/value storage API from what is out there already, maybe we could encapsulate it somewhere that doesn't add newly working public API surface?

NickGerleman avatar Oct 13 '22 08:10 NickGerleman

I can't seem to make this a draft, but FYI it's a draft for now

rbalicki2 avatar Oct 17 '22 20:10 rbalicki2

@rbalicki2 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot avatar Oct 18 '22 06:10 facebook-github-bot

This pull request was successfully merged by @rbalicki2 in 0fac9817df403e31d8256befe52409c948614706.

When will my fix make it into a release? | Upcoming Releases

react-native-bot avatar Oct 26 '22 04:10 react-native-bot

This pull request has been reverted by 51d14b9dbdba0ae978a61aecdfdce3142482e9f9.

facebook-github-bot avatar Oct 26 '22 19:10 facebook-github-bot