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

[react devtools] Device storage support #25213

Closed
wants to merge 1 commit into from
Closed

[react devtools] Device storage support #25213

wants to merge 1 commit into from

Conversation

rbalicki2
Copy link
Contributor

@rbalicki2 rbalicki2 commented Sep 8, 2022

Summary

See this react native diff as well

  • There is a new method on the __REACT_DEVTOOLS_GLOBAL_HOOK__, injectDeviceStorageMethods, which (in this diff) takes { setValueOnDevice }. setValueOnDevice gives devtools the ability to update values in device storage. In a future diff, injectDeviceStorageMethods will take an object with multiple properties.
  • React Native, during runApplication, will set window.__REACT_DEVTOOLS_APPEND_COMPONENT_STACK__ (etc) from the values in device storage, and then call injectDeviceStorageMethods.
  • injectDeviceStorageMethods will:
    • write any recently-changed settings to device storage
    • call patchConsole
  • When a relevant setting changes, we also write it local storage.
  • Thus, when we modify a setting and restart, that setting can be read before the react devtools frontend connects.

Also

  • Instead of calling patchConsole with values from window in multiple places, centralize that in one file

Bigger picture

Three things stand out as obvious follow ups:

  • It's very awkward that react native needs to know to set __REACT_DEVTOOLS_APPEND_COMPONENT_STACK__ from a given key in storage. It would be better if all of that logic could exist on the react devtools side. This can be achieved by injecting { setValueOnDevice, getValueOnDevice }
  • Likewise, it's awkward that when a setting changes (e.g. append component stack), we know to store it in device storage. This code should be centralized in a follow up.
  • This mechanism can be used for component filters, reload-and-profile, etc.

Notes

How did you test this change?

  • Manual testing
  • yarn run prettier, flow dom, and unit tests

@lunaruan
Copy link
Contributor

lunaruan commented Sep 9, 2022

It looks like in your diff, you are putting most of the device storage logic inside the shared devtools folder (react-devtools-shared). This folder is used by the the React DevTools extension, the React DevTools inline package, and React DevTools core and we only need to store settings for React Native.

Looking at your PRs, it seems like the core of the device storage logic lives in React Native, and you are just calling into it from DevTools. I wonder if it would simplify your PR if you moved let RN and DevTools communicate via a bridge instead of passing around functions. Ex:

  • In React Native, when we connect to DevTools, listen to a react-devtools event from the __REACT_DEVTOOLS_GLOBAL_HOOK__ when we set up React DevTools
  • Emit an event from React Native that to tell React DevTools the device settings (which React DevTools will use if they don't have local storage)
  • In DevTools, whenever the settings change, emit an event that React Native will listen to to store the events to global storage?

Also open to other ideas!

@rbalicki2
Copy link
Contributor Author

Yeah, I can move this logic to to connectToDevTools (called from that RN file to which you linked). I'll do this as follows:

  • in connectToDevTools (or at the top level in that file) before the socket is opened, read from local storage and update window.
  • In connectToDevTools, after the socket is opened and we have created an agent, add listeners which will write those values to device storage.

Optimistically, this means we can make no changes to RN. However, maybe including an external library in React DevTools that requires pod install (etc) will be problematic, so I might pass getters/setters over in ConnectOptions

Closing for now, until I make these changes

@rbalicki2 rbalicki2 closed this Sep 9, 2022
@rbalicki2
Copy link
Contributor Author

rbalicki2 commented Sep 9, 2022

Welp, unless we make react-devtools-core have a dependency on react-native, we can't use react-native-mmkv directly from react-devtools-core. I'll go with passing it over.

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