First-party redux support #931
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
maciekstosio
changed the title
| this.devtoolsAgent._bridge.addListener(this.scope, this.handleMessages); | ||
| }; | ||
|
|
||
| clearDevToolsAgent = () => { |
There was a problem hiding this comment.
The pattern we are using in most of the codebase is for setter function to return dispose function that is added to disposables that are called after object destruction , this solution is advantageous, because it does not really on this.scope and this. handleMessages being stable, more over it would be consistant with the rest of the codebase
There was a problem hiding this comment.
Tbh, I'm not sure if I see any benefit in it in this. I can't get rid of this.handleMessages as this.clearDevToolsAgent is called also in closeAsync, if it would be just for set/clearDevToolsAgent I totally get it.
| this.listeners.set(type, [...currentListeners, listener]); | ||
| }; | ||
|
|
||
| removeMessageListener = (type, listener) => { |
There was a problem hiding this comment.
this method seems to never be used
There was a problem hiding this comment.
We're implementing methods that could be used by the redux-devtools-expo-dev-plugin - those method match expo interface.
|
|
||
| export const isProxyClientReady = () => proxyClient !== null; | ||
|
|
||
| export const clearProxyClient = () => proxyClient = null; |
There was a problem hiding this comment.
shouldn't this method call closeAsync ?
There was a problem hiding this comment.
Get rid of that in 0ede424
|
|
||
| export const clearProxyClient = () => proxyClient = null; | ||
|
|
||
| export const createRNIDEProxyClientAsync = async () => { |
There was a problem hiding this comment.
Just to follow types from redux-devtools-expo-dev-plugin
| const { useEffect } = require("react"); | ||
| const {createComposeWithDevTools} = require('./external/redux-devtools-expo-dev-plugin'); | ||
|
|
||
| class RNIDEProxyClient { |
There was a problem hiding this comment.
nit:
This name seems almost too generic i'm not sure what it does from the name
There was a problem hiding this comment.
Changed in: 0ede424
|
|
||
| let proxyClient = null; | ||
|
|
||
| export const isProxyClientReady = () => proxyClient !== null; |
There was a problem hiding this comment.
Is there ever a situation in which ProxyClient exist but is not ready? if not i would rename this method to `available if it is possible it should be checked here
There was a problem hiding this comment.
Yeah, If there isn't redux, or if redux devtools have different configuration. Basically when REDUX_DEVTOOLS_EXTENSION_COMPOSE is not used.
| return () => { | ||
| clearProxyClient(); |
There was a problem hiding this comment.
is it called when the user opens preview? could you test that and check if it does not cause multiple proxies being created?
There was a problem hiding this comment.
I removed that in 0ede424
| @@ -198,6 +198,11 @@ | |||
| "id": "RadonIDEToolExpoDevPluginReduxDevTools", | |||
There was a problem hiding this comment.
why don't we retire the old panel?
There was a problem hiding this comment.
We attach to redux like default dev tools on the web, thus if someone have custom configuration, i.e. using dev-plugins, our will stop working. I could try overriding the config, but I'm unsure if I won't break certain setups. So I'd leave it.
There was a problem hiding this comment.
I was able to reproduce the behavior in which booth panels are open and only one works:
To achieve that just comment and uncomment configuration without restarting the ide:
const store = configureStore({
reducer: rootReducer,
// devTools: false,
// enhancers: (getDefaultEnhancers) =>
// getDefaultEnhancers().concat(devToolsEnhancer()),
});
I don't mind leaving the old panel alive if it is the most straight forward solution but we need a way of establishing which one is used and killing the other one.
There was a problem hiding this comment.
After additional thought this edge case might be adressed in followup review
There was a problem hiding this comment.
There was a problem hiding this comment.
why this file has suck a strange name is it commit name? wouldn't it be better to use tags as a naming convention?
There was a problem hiding this comment.
This is the output from expo export. I don't see a reason to configure a custom build unless the fact that I'm getting those filenames using gs can introduce some problems (?). I assume, the hash at the end is to make sure browser will refreash cache, but that's just my guess.
| viewIdPrefix: "RNIDE.Tool.ExpoDevPlugin.ReactQuery", | ||
| }, | ||
| "@dev-plugins/react-native-mmkv": { | ||
| label: "MMKV", | ||
| label: "MMKV DevPlugin", | ||
| viewIdPrefix: "RNIDE.Tool.ExpoDevPlugin.MMKV", | ||
| }, | ||
| "redux-devtools-expo-dev-plugin": { |
There was a problem hiding this comment.
why don't we retire it?
There was a problem hiding this comment.
We attach to redux like default dev tools on the web, thus if someone have custom configuration, i.e. using dev-plugins, our will stop working. I could try overriding the config, but I'm unsure if I won't break certain setups. So I'd leave it.
| @@ -198,6 +198,11 @@ | |||
| "id": "RadonIDEToolExpoDevPluginReduxDevTools", | |||
There was a problem hiding this comment.
I was able to reproduce the behavior in which booth panels are open and only one works:
To achieve that just comment and uncomment configuration without restarting the ide:
const store = configureStore({
reducer: rootReducer,
// devTools: false,
// enhancers: (getDefaultEnhancers) =>
// getDefaultEnhancers().concat(devToolsEnhancer()),
});
I don't mind leaving the old panel alive if it is the most straight forward solution but we need a way of establishing which one is used and killing the other one.
In #931 we introduces a buil-in support for redux plugin that doesn't rely on expo dev plugins package. This PR however brought a few potential issues to the codebase, so here we're refactoring it aiming to fix those problems while also preparing for more steamlined support for other tools: 1) removing console log statements introduced in #931 that were polluting the app output 2) clearing up redux registration such that it only relies on the "compose" callback and doesn't require subsequent calls for setting devtools agent nor hook-based integration. This is done by accessing the devtools agent in the same way we do that in the wrapper component. 3) getting rid of direct imports of devtools related code – this for unknown reason turns out to be causing issues with metro as direct imports would impact the order in which modules are loaded and hence may impact the app behavior. 4) we're refactoring module registration such that it is no longer specific to expo. Before we had registry for expo-related plugins and now in #931 a separate registration process was introduced for redux plugin specifically. We're making it more generic and porting both expo plugins and the redux plugin to use it. ### How Has This Been Tested: 1. Fire up example app with plugins 2. Test redux plugin shows up on the list when redux is used and doesn't show up otherwise. 3. Test scenario from #931
This PR adds first-party react-query dev tools support. It works without any additional configuration and does not conflict with the Expo Dev plugin, so the old implementation, which is based on Expo Dev Plugins (#878), is being removed. **The solution works as follows:** - While transforming the react-query library, we inject a code that calls `window.__RNIDE_REACT_QUERY_CLIENT_INIT__` with `queryClient` on mount; - `__RNIDE_REACT_QUERY_CLIENT_INIT__` broadcasts the apps react-query state using RN dev tools; - Radon IDE extension is used as a proxy - receives the events from RN dev-tools and sends them to React Query DevTools webview; - Thanks to the above, webview has synced react-query state and displays it using [original react-query devtools](https://github.com/TanStack/query/tree/main/packages/query-devtools). **WebUI** WebUI needs to be bundled so it can be inserted into webview. For that, I created a separate repo https://github.com/software-mansion-labs/radon-ide-react-query-webui - which consists of https://github.com/TanStack/query/tree/main/packages/query-devtools with additional logic required for syncing the state between app and dev tools. Built artifacts of webui are committed to the webui repo and are copied while building the Radon IDE extension. **Communication** The reason is to sync both states (apps with dev tools one). The sync is based on https://github.com/TanStack/query/blob/main/packages/query-broadcast-client-experimental/src/index.ts, but instead of `BroadcastChannel`, we communicate through RN dev-tools similarly to redux dev tools #931. ### How Has This Been Tested: - run `eas-52-prebuild-with-plugins` - check if `React Query DevTools`, are available in tools list - make sure `React Query DevTools DevPlugin` is not visible - open `React Query DevTools` - go to plugins page - press React Query counter - confirm that the events are visible
This PR adds first-party support for redux. I resued the enhancer part and WebUI from https://github.com/matt-oakes/redux-devtools-expo-dev-plugin - the code seems to do everything we need with nicely separated communication, except that the communication through the
expo/devtoolsis hardcode. I created a fork of it that's included as a submodule in our repo, but I'm trying to upstream the changes with communication logic abstraction (matt-oakes/redux-devtools-expo-dev-plugin#19) so we can easily keep the fork up to date. The rest (communication through vscode) can be found in software-mansion-labs/redux-devtools-expo-dev-plugin#2).To archive zero-config support, we utilize
window.__REDUX_DEVTOOLS_EXTENSION_COMPOSE__, where we pass compose fromredux-devtools-expo-dev-pluginwith our communication injected. Redux dev tools communicate with the extension through the dev tools, and the extension passes messages through webview events to WebUI.We need to have
redux-devtools-expo-dev-pluginbuilt to import through a wrapper in app runtime. Similarly, the webui needs to be bundled, thus I addedpackages/vscode-extension/scripts/install-dev-plugins.sh, currently the product of it is committed, and it needs to be run manually, but this is subjected to change.How Has This Been Tested:
expo-52-prebuild-with-plugins