Create abstraction for communication#19
Create abstraction for communication#19maciekstosio wants to merge 4 commits intomatt-oakes:mainfrom
Conversation
…-abstraction-for-proxy-client Create abstraction for proxy client
|
Thanks for the contribution! To just make this a little easier to review, can you send over a description of the changes you have made and how they help solve your issue. Can you also let me know the reasoning behind changing Radon IDE looks great by the way! I'll have to check it out next time I'm working on an RN/Expo project :) |
There was a problem hiding this comment.
Hi, thanks for taking a look. I added some inline comments, it should help follow what happens, and why. In case of any further questions let me know, and I'll try to explain it better.
In general, the idea is that the library provides both the middleware part and webui nicely separated except for the getDevToolsPluginClientAsync hardcoded. Abstracting it can help us simply inject our custom communication, and at the same time, it sounds like something that shouldn't be harmful to the library itself. Abstraction will allow us to follow changes and, if needed, improve one library without a bunch of conflicts. We provide two different communication mechanisms for webui (https://github.com/software-mansion-labs/redux-devtools-expo-dev-plugin/pull/2/files) and for the middleware part (packages/vscode-extension/lib/plugins/redux-devtools.js).
Definitely, take a Radon IDE for a spin! If you have any thoughts, problems, or feedback, just reach out to me or post an issue in our repo!
| filters: LocalFilter | undefined; | ||
| instanceId?: string; | ||
| devToolsPluginClient?: DevToolsPluginClient; | ||
| proxyClient?: ProxyClient; |
There was a problem hiding this comment.
The main idea is that we abstract communication logic. As for now, the getDevToolsPluginClientAsync and DevToolsPluginClient are hardcoded and tidly coupled. I wanted to allow to pass any method that returns a subset of DevToolsPluginClient that is used by redux-devtools-expo-dev-plugin. I assume that if the library wants to use more features of DevToolsPluginClient, then ProxyClient should be updated.
|
|
||
| export function composeWithDevTools( | ||
| ...funcs: [Options<unknown, Action<string>>] | StoreEnhancer[] | ||
| export function createComposeWithDevTools( |
There was a problem hiding this comment.
I changed composeWithDevTools to createComposeWithDevTools just to allow passing another communication factory. It should create what used to be composeWithDevTools. I need to have composeWithDevTools, with our communication client injected, to provide zero config setup for radon ide: https://github.com/software-mansion/radon-ide/blob/497669db377caf1ba8adca62a1ffafa88a1441d1/packages/vscode-extension/lib/plugins/redux-devtools.js#L75
| if (process.env.NODE_ENV !== "production") { | ||
| devtoolsEnhancer = require("./devtools").default; | ||
| composeWithDevTools = require("./devtools").composeWithDevTools; | ||
| const getDevToolsPluginClientAsync = require('expo/devtools').getDevToolsPluginClientAsync; |
There was a problem hiding this comment.
I moved the import of getDevToolsPluginClientAsync to the index so it's easier to replace the communication mechanism, and omit it during bundling: https://github.com/software-mansion-labs/redux-devtools-expo-dev-plugin/pull/2/files#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80
| import { nonReduxDispatch } from "../utils/monitorActions"; | ||
|
|
||
| let devToolsPluginClient: DevToolsPluginClient | undefined; | ||
| let proxyClient: ProxyClient | undefined; |
There was a problem hiding this comment.
Similarly, for the redux middleware part, I added an abstraction. Just, to allow using a different method that has the same interface as getDevToolsPluginClientAsync
| store = inStore; | ||
| connect(); | ||
| export function api( | ||
| createProxyClient: ProxyClientFactory = () => getDevToolsPluginClientAsync("redux-devtools-expo-dev-plugin"), |
There was a problem hiding this comment.
By default it is still using getDevToolsPluginClientAsync. But on our branch I provide custom communication mechanism: https://github.com/software-mansion-labs/redux-devtools-expo-dev-plugin/pull/2/files#diff-cf56f00851c2aa7b536d9a15a35751d6ce4511a7b2ba5f353e059b6d0ba4dad4
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/devtools` is 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 from `redux-devtools-expo-dev-plugin` with 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-plugin` built to import through a wrapper in app runtime. Similarly, the webui needs to be bundled, thus I added `packages/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: - Open `expo-52-prebuild-with-plugins` - Comment out configuration related to expo dev plugins ``` const store = configureStore({ reducer: rootReducer, // devTools: false, // enhancers: (getDefaultEnhancers) => // getDefaultEnhancers().concat(devToolsEnhancer()), }); ``` - Reload the app - Confirm redux dev tools work
2 participants
This PRs adds abstraction to
redux-devtools-expo-devplugin. This change will help separate the communication logic and the devtools part itself. I wanted to reuse the library for Radon IDE, but I need a different way to communicate thanexpo/devtools. It shouldn't impact the behavior ofredux-devtools-epo-dev-pluginin any way, we will keep our implementation on our fork. The abstraction is added on both "sides": in the dev tools webui and redux enhancer/compose.How it has been tested: