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

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

Closed
wants to merge 1 commit into from

Conversation

rbalicki2
Copy link
Contributor

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

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Facebook Partner: Facebook Partner labels Oct 13, 2022
@facebook-github-bot
Copy link
Contributor

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

@react-native-bot react-native-bot added the Type: Enhancement A new feature or enhancement of an existing feature. label Oct 13, 2022
@analysis-bot
Copy link

analysis-bot commented Oct 13, 2022

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: 31f2199
Branch: main

@analysis-bot
Copy link

analysis-bot commented Oct 13, 2022

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

Base commit: 31f2199
Branch: main

@NickGerleman
Copy link
Contributor

NickGerleman commented Oct 13, 2022

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?

@rbalicki2 rbalicki2 force-pushed the file-storage branch 11 times, most recently from 97fd1a9 to 0a4d713 Compare October 17, 2022 20:10
@rbalicki2
Copy link
Contributor Author

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

@rbalicki2 rbalicki2 force-pushed the file-storage branch 2 times, most recently from 0a4d713 to 8b655ca Compare October 17, 2022 21:59
* @format
*/

import Settings from './Settings';

Choose a reason for hiding this comment

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

  • ⚠️ Libraries/Settings/DevToolsSettingsManager.ios.js line 11 – Requires should be sorted alphabetically (lint/sort-imports)

@rbalicki2 rbalicki2 force-pushed the file-storage branch 7 times, most recently from 6081537 to 4ceb8eb Compare October 18, 2022 05:23
@facebook-github-bot
Copy link
Contributor

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

@react-native-bot
Copy link
Collaborator

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

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

@react-native-bot react-native-bot added the Merged This PR has been merged. label Oct 26, 2022
@facebook-github-bot
Copy link
Contributor

This pull request has been reverted by 51d14b9.

rbalicki2 pushed a commit to rbalicki2/react-native that referenced this pull request Nov 1, 2022
Summary:
# What

This diff contains all the changes from D40333083 (facebook@0fac981) (aka facebook#34964), **except** the change to `setUpReactDevTools.js`, which actually uses the new files.

# Why

* We want to ship the Buck, C++, etc. changes before the JavaScript changes that depend on those files.
* Otherwise, apps can fail at startup with the message:
```
`TurboModuleRegistry.getEnforcing(...): '${name}' could not be found. ` +
      'Verify that a module by this name is registered in the native binary.',
```
* Note that this only occurs if you are using a previously-built version of the C++, Obj C, etc. files in RN, but a more recent version of the JavaScript files. If you are building from matching sources, this does not occur.
* After a few days, we can land the JS files.

Changelog
[General][Added] Add, but don't use, DevTools Settings Manager.

Differential Revision: D40873390

fbshipit-source-id: 9eff55ead8f574ef8def506369c2e28219aa2ce0
rbalicki2 pushed a commit to rbalicki2/react-native that referenced this pull request Nov 1, 2022
…acebook#35163)

Summary:
Pull Request resolved: facebook#35163

# What

This diff contains all the changes from D40333083 (facebook@0fac981) (aka facebook#34964), **except** the change to `setUpReactDevTools.js`, which actually uses the new files.

# Why

* We want to ship the Buck, C++, etc. changes before the JavaScript changes that depend on those files.
* Otherwise, apps can fail at startup with the message:
```
`TurboModuleRegistry.getEnforcing(...): '${name}' could not be found. ` +
      'Verify that a module by this name is registered in the native binary.',
```
* Note that this only occurs if you are using a previously-built version of the C++, Obj C, etc. files in RN, but a more recent version of the JavaScript files. If you are building from matching sources, this does not occur.
* After a few days, we can land the JS files.

## Changelog
[General][Added] Add, but don't use, DevTools Settings Manager.

Differential Revision: D40873390

fbshipit-source-id: 510fa3accb2a9037cd365adad6ba0c9d87d360a8
rbalicki2 pushed a commit to rbalicki2/react-native that referenced this pull request Nov 1, 2022
…acebook#35163)

Summary:
Pull Request resolved: facebook#35163

# What

This diff contains all the changes from D40333083 (facebook@0fac981) (aka facebook#34964), **except** the change to `setUpReactDevTools.js`, which actually uses the new files.

# Why

* We want to ship the Buck, C++, etc. changes before the JavaScript changes that depend on those files.
* Otherwise, apps can fail at startup with the message:
```
`TurboModuleRegistry.getEnforcing(...): '${name}' could not be found. ` +
      'Verify that a module by this name is registered in the native binary.',
```
* Note that this only occurs if you are using a previously-built version of the C++, Obj C, etc. files in RN, but a more recent version of the JavaScript files. If you are building from matching sources, this does not occur.
* After a few days, we can land the JS files.

## Changelog
[General][Added] Add, but don't use, DevTools Settings Manager.

Differential Revision: D40873390

fbshipit-source-id: 4d401f7d0e5051eb25ed984d814d8effd9ad2915
facebook-github-bot pushed a commit that referenced this pull request Nov 7, 2022
…35163)

Summary:
Pull Request resolved: #35163

# What

This diff contains all the changes from D40333083 (0fac981) (aka #34964), **except** the change to `setUpReactDevTools.js`, which actually uses the new files.

# Why

* We want to ship the Buck, C++, etc. changes before the JavaScript changes that depend on those files.
* Otherwise, apps can fail at startup with the message:
```
`TurboModuleRegistry.getEnforcing(...): '${name}' could not be found. ` +
      'Verify that a module by this name is registered in the native binary.',
```
* Note that this only occurs if you are using a previously-built version of the C++, Obj C, etc. files in RN, but a more recent version of the JavaScript files. If you are building from matching sources, this does not occur.
* After a few days, we can land the JS files.

## Changelog

Changelog
[General][Added] Add, but don't use, DevTools Settings Manager.

Reviewed By: NickGerleman

Differential Revision: D40873390

fbshipit-source-id: c7bac6ae65f85666b8616443db278ebb175b691b
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
* Add a DevToolsSettingsManager, which has android and iOS variants, which uses a new TM (Android) or takes advantage of the Settings TM (iOS) to get/set console patch settings
* This is backed by either the existing Settings module (iOS) or a new Java TM, which uses the SharedPreferences AP

## Testing

Manual testing

## Changelog

[General] [Added] - Add DevToolsSettingsManager

Pull Request resolved: facebook#34964

Test Plan: * Extensive manual testing

Reviewed By: NickGerleman

Differential Revision: D40333083

Pulled By: rbalicki2

fbshipit-source-id: f3816e3bd7dea3086f6f2269c3a099af14aebb3b
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
…acebook#35163)

Summary:
Pull Request resolved: facebook#35163

# What

This diff contains all the changes from D40333083 (facebook@0fac981) (aka facebook#34964), **except** the change to `setUpReactDevTools.js`, which actually uses the new files.

# Why

* We want to ship the Buck, C++, etc. changes before the JavaScript changes that depend on those files.
* Otherwise, apps can fail at startup with the message:
```
`TurboModuleRegistry.getEnforcing(...): '${name}' could not be found. ` +
      'Verify that a module by this name is registered in the native binary.',
```
* Note that this only occurs if you are using a previously-built version of the C++, Obj C, etc. files in RN, but a more recent version of the JavaScript files. If you are building from matching sources, this does not occur.
* After a few days, we can land the JS files.

## Changelog

Changelog
[General][Added] Add, but don't use, DevTools Settings Manager.

Reviewed By: NickGerleman

Differential Revision: D40873390

fbshipit-source-id: c7bac6ae65f85666b8616443db278ebb175b691b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. p: Facebook Partner: Facebook Partner Reverted Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants