From 04fcaddded94bd7a0436e18acc2946b53e5df49e Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Thu, 2 Jan 2020 07:12:06 -0800 Subject: [PATCH] Only store messages for plugins that are starred Summary: From several reviews / early feedbacks it was suggested several times to use the star mechanism to distinguish which plugins are allowed to send messages. This diff implements that: - If a plugin is not selected, and not starred, it will drop the messages it received in the background - This logic is still behind the same GK - I think this change warrants upping the message queue limits significantly - A future optimization would be to disable sending messages from the device side of things to reduce bridge usage, but that change is probably a lot more complicated with less impact - In the next diff I'll make clear from UI perspective that unstarred plugins don't queue messages In the attach video one can see how graphQL plugin keeps storing messages if it is starred, but if it isn't starred and not selected either, it will skip messages {F225692819} Reviewed By: jknoxville Differential Revision: D19250928 fbshipit-source-id: 7e6af0eb6830dc79951cfd206e05b44061f1b14a --- .../createMockFlipperWithPlugin.node.tsx.snap | 14 ++- .../createMockFlipperWithPlugin.tsx | 30 ++++- src/utils/__tests__/messageQueue.node.tsx | 105 +++++++++++------- src/utils/messageQueue.tsx | 66 ++++++----- src/utils/pluginUtils.tsx | 6 +- 5 files changed, 140 insertions(+), 81 deletions(-) diff --git a/src/__tests__/__snapshots__/createMockFlipperWithPlugin.node.tsx.snap b/src/__tests__/__snapshots__/createMockFlipperWithPlugin.node.tsx.snap index 627a75731..954a5a4a4 100644 --- a/src/__tests__/__snapshots__/createMockFlipperWithPlugin.node.tsx.snap +++ b/src/__tests__/__snapshots__/createMockFlipperWithPlugin.node.tsx.snap @@ -21,7 +21,7 @@ Object { "logs": Array [], "os": "Android", "serial": "serial", - "title": "title", + "title": "MockAndroidDevice", }, ], "errors": Array [], @@ -31,15 +31,19 @@ Object { "logs": Array [], "os": "Android", "serial": "serial", - "title": "title", + "title": "MockAndroidDevice", }, "selectedPlugin": "TestPlugin", "staticView": null, "uninitializedClients": Array [], - "userPreferredApp": null, - "userPreferredDevice": null, + "userPreferredApp": "TestApp#Android#unit_test#serial", + "userPreferredDevice": "MockAndroidDevice", "userPreferredPlugin": "TestPlugin", - "userStarredPlugins": Object {}, + "userStarredPlugins": Object { + "TestApp": Array [ + "TestPlugin", + ], + }, } `; diff --git a/src/test-utils/createMockFlipperWithPlugin.tsx b/src/test-utils/createMockFlipperWithPlugin.tsx index bdf971b70..304eba665 100644 --- a/src/test-utils/createMockFlipperWithPlugin.tsx +++ b/src/test-utils/createMockFlipperWithPlugin.tsx @@ -9,7 +9,12 @@ import {createStore} from 'redux'; -import {selectPlugin} from '../reducers/connections'; +import { + selectPlugin, + starPlugin, + selectDevice, + selectClient, +} from '../reducers/connections'; import BaseDevice from '../devices/BaseDevice'; import reducers, {Store} from '../reducers/index'; @@ -38,7 +43,12 @@ export async function createMockFlipperWithPlugin( }) => Promise, ) { const store = createStore(reducers); - const device = new BaseDevice('serial', 'physical', 'title', 'Android'); + const device = new BaseDevice( + 'serial', + 'physical', + 'MockAndroidDevice', + 'Android', + ); const logger = createStubLogger(); store.dispatch(registerPlugins([pluginClazz])); @@ -67,7 +77,7 @@ export async function createMockFlipperWithPlugin( null, // create a stub connection to avoid this plugin to be archived? logger, store, - [pluginClazz.name], + [pluginClazz.id], device, ); @@ -79,14 +89,26 @@ export async function createMockFlipperWithPlugin( }, } as any; + // As convenience, by default we select the new client, star the plugin, and select it store.dispatch({ type: 'NEW_CLIENT', payload: client, }); + store.dispatch(selectDevice(device)); + + store.dispatch(selectClient(client.id)); + + store.dispatch( + starPlugin({ + selectedPlugin: pluginClazz.id, + selectedApp: client.query.app, + }), + ); + store.dispatch( selectPlugin({ - selectedPlugin: pluginClazz.name, + selectedPlugin: pluginClazz.id, selectedApp: client.query.app, deepLinkPayload: null, }), diff --git a/src/utils/__tests__/messageQueue.node.tsx b/src/utils/__tests__/messageQueue.node.tsx index 9f20749b5..d32829e6c 100644 --- a/src/utils/__tests__/messageQueue.node.tsx +++ b/src/utils/__tests__/messageQueue.node.tsx @@ -9,8 +9,8 @@ import {FlipperPlugin} from '../../plugin'; import {createMockFlipperWithPlugin} from '../../test-utils/createMockFlipperWithPlugin'; -import {GK} from 'flipper'; -import {selectPlugin} from '../../reducers/connections'; +import {GK, Store, Client} from 'flipper'; +import {selectPlugin, starPlugin} from '../../reducers/connections'; import {processMessageQueue} from '../messageQueue'; import {getPluginKey} from '../pluginUtils'; import {TestIdler} from '../Idler'; @@ -48,6 +48,35 @@ class TestPlugin extends FlipperPlugin { } } +function starTestPlugin(store: Store, client: Client) { + store.dispatch( + starPlugin({ + selectedPlugin: TestPlugin.id, + selectedApp: client.query.app, + }), + ); +} + +function selectDeviceLogs(store: Store) { + store.dispatch( + selectPlugin({ + selectedPlugin: 'DeviceLogs', + selectedApp: null, + deepLinkPayload: null, + }), + ); +} + +function selectTestPlugin(store: Store, client: Client) { + store.dispatch( + selectPlugin({ + selectedPlugin: TestPlugin.id, + selectedApp: client.query.app, + deepLinkPayload: null, + }), + ); +} + test('will process event with GK disabled', async () => { await createMockFlipperWithPlugin( TestPlugin, @@ -87,18 +116,12 @@ test('queue - events are processed immediately if plugin is selected', async () ); }); -test('queue - events are NOT processed immediately if plugin is NOT selected', async () => { +test('queue - events are NOT processed immediately if plugin is NOT selected (but starred)', async () => { await createMockFlipperWithPlugin( TestPlugin, async ({client, device, store, sendMessage}) => { await GK.withWhitelistedGK('flipper_event_queue', async () => { - store.dispatch( - selectPlugin({ - selectedPlugin: 'DeviceLogs', - selectedApp: null, - deepLinkPayload: null, - }), - ); + selectDeviceLogs(store); expect(store.getState().connections.selectedPlugin).not.toBe( 'TestPlugin', ); @@ -137,6 +160,32 @@ test('queue - events are NOT processed immediately if plugin is NOT selected', a expect(store.getState().pluginMessageQueue).toEqual({ [pluginKey]: [], }); + + // unstar, but, messages still arrives because selected + starTestPlugin(store, client); + selectTestPlugin(store, client); + sendMessage('inc', {delta: 3}); + // active, immediately processed + expect(store.getState().pluginStates).toEqual({ + [pluginKey]: { + count: 6, + }, + }); + + // different plugin, and not starred, message will never arrive + selectDeviceLogs(store); + sendMessage('inc', {delta: 4}); + expect(store.getState().pluginMessageQueue).toEqual({ + [pluginKey]: [], + }); + + // star again, plugin still not selected, message is queued + starTestPlugin(store, client); + sendMessage('inc', {delta: 5}); + + expect(store.getState().pluginMessageQueue).toEqual({ + [pluginKey]: [{method: 'inc', params: {delta: 5}}], + }); }); }, ); @@ -147,14 +196,7 @@ test('queue - events processing will be paused', async () => { TestPlugin, async ({client, device, store, sendMessage}) => { await GK.withWhitelistedGK('flipper_event_queue', async () => { - // select a different plugin - store.dispatch( - selectPlugin({ - selectedPlugin: 'DeviceLogs', - selectedApp: null, - deepLinkPayload: null, - }), - ); + selectDeviceLogs(store); sendMessage('inc', {}); sendMessage('inc', {delta: 3}); @@ -208,14 +250,7 @@ test('queue - messages that arrive during processing will be queued', async () = TestPlugin, async ({client, device, store, sendMessage}) => { await GK.withWhitelistedGK('flipper_event_queue', async () => { - // select a different plugin - store.dispatch( - selectPlugin({ - selectedPlugin: 'DeviceLogs', - selectedApp: null, - deepLinkPayload: null, - }), - ); + selectDeviceLogs(store); sendMessage('inc', {}); sendMessage('inc', {delta: 2}); @@ -269,14 +304,7 @@ test('queue - processing can be cancelled', async () => { TestPlugin, async ({client, device, store, sendMessage}) => { await GK.withWhitelistedGK('flipper_event_queue', async () => { - // select a different plugin - store.dispatch( - selectPlugin({ - selectedPlugin: 'DeviceLogs', - selectedApp: null, - deepLinkPayload: null, - }), - ); + selectDeviceLogs(store); sendMessage('inc', {}); sendMessage('inc', {delta: 2}); @@ -317,14 +345,7 @@ test('queue - make sure resetting plugin state clears the message queue', async TestPlugin, async ({client, device, store, sendMessage}) => { await GK.withWhitelistedGK('flipper_event_queue', async () => { - // select a different plugin - store.dispatch( - selectPlugin({ - selectedPlugin: 'DeviceLogs', - selectedApp: null, - deepLinkPayload: null, - }), - ); + selectDeviceLogs(store); sendMessage('inc', {}); sendMessage('inc', {delta: 2}); diff --git a/src/utils/messageQueue.tsx b/src/utils/messageQueue.tsx index feefe689d..d57b3516b 100644 --- a/src/utils/messageQueue.tsx +++ b/src/utils/messageQueue.tsx @@ -7,7 +7,7 @@ * @format */ -import {PersistedStateReducer} from '../plugin'; +import {PersistedStateReducer, FlipperDevicePlugin} from '../plugin'; import {State, MiddlewareAPI} from '../reducers/index'; import {setPluginState} from '../reducers/pluginStates'; import {flipperRecorderAddEvent} from './pluginStateRecorder'; @@ -18,6 +18,7 @@ import { } from '../reducers/pluginMessageQueue'; import {Idler, BaseIdler} from './Idler'; import {getPluginKey} from './pluginUtils'; +import {deconstructClientId} from './clientUtils'; const MAX_BACKGROUND_TASK_TIME = 25; @@ -137,35 +138,46 @@ export function processMessageLater( }, message: {method: string; params?: any}, ) { - // TODO: can we make this better? - const selection = store.getState().connections; - const selectedPlugin = - selection.selectedPlugin && - getPluginKey( - selection.selectedApp, - selection.selectedDevice, - selection.selectedPlugin, - ); - // if the plugin is active, and has no queued messaged, process the message immediately - if ( - selectedPlugin === pluginKey && - getPendingMessages(store, pluginKey).length === 0 - ) { - processMessageImmediately(store, pluginKey, plugin, message); - } else { - // TODO: possible optimization: drop all messages for non-favorited plugins - // TODO: possible optimization: drop messages if queue is too large - store.dispatch( - queueMessage( - pluginKey, - message.method, - message.params, - plugin.maxQueueSize, - ), - ); + const isSelected = pluginKey === getSelectedPluginKey(store.getState()); + switch (true) { + case isSelected && getPendingMessages(store, pluginKey).length === 0: + processMessageImmediately(store, pluginKey, plugin, message); + break; + case isSelected: + case plugin instanceof FlipperDevicePlugin: + case pluginIsStarred(store.getState(), plugin.id): + store.dispatch( + queueMessage( + pluginKey, + message.method, + message.params, + plugin.maxQueueSize, + ), + ); + break; } } +function getSelectedPluginKey(state: State): string | undefined { + return state.connections.selectedPlugin + ? getPluginKey( + state.connections.selectedApp, + state.connections.selectedDevice, + state.connections.selectedPlugin, + ) + : undefined; +} + +function pluginIsStarred(state: State, pluginId: string): boolean { + const {selectedApp} = state.connections; + if (!selectedApp) { + return false; + } + const appInfo = deconstructClientId(selectedApp); + const starred = state.connections.userStarredPlugins[appInfo.app]; + return starred && starred.indexOf(pluginId) > -1; +} + export async function processMessageQueue( plugin: { defaultPersistedState: any; diff --git a/src/utils/pluginUtils.tsx b/src/utils/pluginUtils.tsx index 70076c02f..2bd032b57 100644 --- a/src/utils/pluginUtils.tsx +++ b/src/utils/pluginUtils.tsx @@ -34,12 +34,12 @@ export function pluginsClassMap( } export function getPluginKey( - selectedApp: string | null, + selectedAppId: string | null, baseDevice: BaseDevice | null, pluginID: string, ): string { - if (selectedApp) { - return `${selectedApp}#${pluginID}`; + if (selectedAppId) { + return `${selectedAppId}#${pluginID}`; } if (baseDevice) { // If selected App is not defined, then the plugin is a device plugin