From fa67c21def463e054c7ec9c09e340821cad7d1bc Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Fri, 10 Dec 2021 17:58:06 -0800 Subject: [PATCH] Fix bug where client plugins weren't always started Summary: Changelog: Fixed issue where occasionally a plugin wouldn't open after starting Flipper This fixes a long standing issue where rarely Flipper wouldn't show the selected plugin. This turned out to be a raise condition, that was easy to reproduce in the Flipper browser version; if a client register before all the plugins are loaded, the plugins that are enabled for that client, but not loaded yet, will not instantiate and hence not show up. This diff fixes that Reviewed By: timur-valiev, aigoncharov Differential Revision: D32987162 fbshipit-source-id: f3179cd9b6f2e4e79d05be1f2236f63acdf50495 --- desktop/flipper-ui-core/src/Client.tsx | 3 +++ .../createMockFlipperWithPlugin.node.tsx.snap | 2 +- .../src/dispatcher/flipperServer.tsx | 10 +++---- .../dispatcher/handleOpenPluginDeeplink.tsx | 18 ++----------- .../src/startFlipperDesktop.tsx | 3 ++- .../src/test-utils/MockFlipper.tsx | 4 ++- desktop/flipper-ui-core/src/utils/waitFor.tsx | 27 +++++++++++++++++++ 7 files changed, 43 insertions(+), 24 deletions(-) create mode 100644 desktop/flipper-ui-core/src/utils/waitFor.tsx diff --git a/desktop/flipper-ui-core/src/Client.tsx b/desktop/flipper-ui-core/src/Client.tsx index f0bbc9dfc..10b32f7e1 100644 --- a/desktop/flipper-ui-core/src/Client.tsx +++ b/desktop/flipper-ui-core/src/Client.tsx @@ -46,6 +46,7 @@ import { isFlipperMessageDebuggingEnabled, registerFlipperDebugMessage, } from './chrome/FlipperMessages'; +import {waitFor} from './utils/waitFor'; type Plugins = Set; type PluginsArr = Array; @@ -173,6 +174,8 @@ export default class Client extends EventEmitter { async init() { await this.loadPlugins(); + // if a client arrives before all plugins are loaded, we'll have to wait + await waitFor(this.store, (state) => state.plugins.initialized); // this starts all sandy enabled plugins this.plugins.forEach((pluginId) => this.startPluginIfNeeded(this.getPlugin(pluginId)), diff --git a/desktop/flipper-ui-core/src/__tests__/__snapshots__/createMockFlipperWithPlugin.node.tsx.snap b/desktop/flipper-ui-core/src/__tests__/__snapshots__/createMockFlipperWithPlugin.node.tsx.snap index 058f4f8ce..51dde958c 100644 --- a/desktop/flipper-ui-core/src/__tests__/__snapshots__/createMockFlipperWithPlugin.node.tsx.snap +++ b/desktop/flipper-ui-core/src/__tests__/__snapshots__/createMockFlipperWithPlugin.node.tsx.snap @@ -84,7 +84,7 @@ Object { "disabledPlugins": Array [], "failedPlugins": Array [], "gatekeepedPlugins": Array [], - "initialized": false, + "initialized": true, "installedPlugins": Map {}, "loadedPlugins": Map {}, "marketplacePlugins": Array [], diff --git a/desktop/flipper-ui-core/src/dispatcher/flipperServer.tsx b/desktop/flipper-ui-core/src/dispatcher/flipperServer.tsx index 9ee7269ba..f6ecfe0e1 100644 --- a/desktop/flipper-ui-core/src/dispatcher/flipperServer.tsx +++ b/desktop/flipper-ui-core/src/dispatcher/flipperServer.tsx @@ -22,6 +22,7 @@ import BaseDevice from '../devices/BaseDevice'; import {ClientDescription, timeout} from 'flipper-common'; import {reportPlatformFailures} from 'flipper-common'; import {sideEffect} from '../utils/sideEffect'; +import {waitFor} from '../utils/waitFor'; export function connectFlipperServerToStore( server: FlipperServer, @@ -112,17 +113,16 @@ export function connectFlipperServerToStore( 'Flipper server started and accepting device / client connections', ); - server - .exec('device-list') + // this flow is spawned delibarately from this main flow + waitFor(store, (state) => state.plugins.initialized) + .then(() => server.exec('device-list')) .then((devices) => { // register all devices devices.forEach((device) => { handleDeviceConnected(server, store, logger, device); }); }) - .then(() => { - return server.exec('client-list'); - }) + .then(() => server.exec('client-list')) .then((clients) => { clients.forEach((client) => { handleClientConnected(server, store, logger, client); diff --git a/desktop/flipper-ui-core/src/dispatcher/handleOpenPluginDeeplink.tsx b/desktop/flipper-ui-core/src/dispatcher/handleOpenPluginDeeplink.tsx index ce3eef431..0e39faa60 100644 --- a/desktop/flipper-ui-core/src/dispatcher/handleOpenPluginDeeplink.tsx +++ b/desktop/flipper-ui-core/src/dispatcher/handleOpenPluginDeeplink.tsx @@ -11,7 +11,7 @@ import React from 'react'; import {Dialog, getFlipperLib} from 'flipper-plugin'; import {isTest} from 'flipper-common'; import {getUser} from '../fb-stubs/user'; -import {State, Store} from '../reducers/index'; +import {Store} from '../reducers/index'; import {checkForUpdate} from '../fb-stubs/checkForUpdate'; import {getAppVersion} from '../utils/info'; import {UserNotSignedInError} from 'flipper-common'; @@ -39,6 +39,7 @@ import { OpenPluginParams, } from '../deeplinkTracking'; import {getRenderHostInstance} from '../RenderHost'; +import {waitFor} from '../utils/waitFor'; export function parseOpenPluginParams(query: string): OpenPluginParams { // 'flipper://open-plugin?plugin-id=graphql&client=facebook&devices=android,ios&chrome=1&payload=' @@ -272,21 +273,6 @@ async function waitForLogin(store: Store) { return waitFor(store, (state) => !!state.user?.id); } -// make this more reusable? -function waitFor( - store: Store, - predicate: (state: State) => boolean, -): Promise { - return new Promise((resolve) => { - const unsub = store.subscribe(() => { - if (predicate(store.getState())) { - unsub(); - resolve(); - } - }); - }); -} - async function verifyFlipperIsUpToDate(title: string) { if (!isProduction() || isTest()) { return; diff --git a/desktop/flipper-ui-core/src/startFlipperDesktop.tsx b/desktop/flipper-ui-core/src/startFlipperDesktop.tsx index 64235fd88..683602980 100644 --- a/desktop/flipper-ui-core/src/startFlipperDesktop.tsx +++ b/desktop/flipper-ui-core/src/startFlipperDesktop.tsx @@ -143,7 +143,6 @@ function init(flipperServer: FlipperServer) { setLoggerInstance(logger); startGlobalErrorHandling(); loadTheme(settings.darkMode); - connectFlipperServerToStore(flipperServer, store, logger); // rehydrate app state before exposing init const persistor = persistStore(store, undefined, () => { @@ -163,6 +162,8 @@ function init(flipperServer: FlipperServer) { } }); + connectFlipperServerToStore(flipperServer, store, logger); + ReactDOM.render( , document.getElementById('root'), diff --git a/desktop/flipper-ui-core/src/test-utils/MockFlipper.tsx b/desktop/flipper-ui-core/src/test-utils/MockFlipper.tsx index 687715d64..b68413f66 100644 --- a/desktop/flipper-ui-core/src/test-utils/MockFlipper.tsx +++ b/desktop/flipper-ui-core/src/test-utils/MockFlipper.tsx @@ -19,7 +19,7 @@ import { ClientResponseType, } from 'flipper-common'; import {PluginDefinition} from '../plugin'; -import {registerPlugins} from '../reducers/plugins'; +import {pluginsInitialized, registerPlugins} from '../reducers/plugins'; import {getLogger} from 'flipper-common'; import {initializeFlipperLibImplementation} from '../utils/flipperLibImplementation'; import pluginManager from '../dispatcher/pluginManager'; @@ -28,6 +28,7 @@ import ArchivedDevice from '../devices/ArchivedDevice'; import {ClientQuery, DeviceOS} from 'flipper-common'; import {TestDevice} from './TestDevice'; import {getRenderHostInstance} from '../RenderHost'; +import {waitFor} from '../utils/waitFor'; export interface AppOptions { plugins?: PluginDefinition[]; @@ -95,6 +96,7 @@ export default class MockFlipper { this._logger, ); this._store.dispatch(registerPlugins(plugins ?? [])); + this._store.dispatch(pluginsInitialized()); } public async initWithDeviceAndClient( diff --git a/desktop/flipper-ui-core/src/utils/waitFor.tsx b/desktop/flipper-ui-core/src/utils/waitFor.tsx new file mode 100644 index 000000000..c0044f9f3 --- /dev/null +++ b/desktop/flipper-ui-core/src/utils/waitFor.tsx @@ -0,0 +1,27 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @format + */ + +import {State, Store} from '../reducers/index'; + +export async function waitFor( + store: Store, + predicate: (state: State) => boolean, +): Promise { + if (predicate(store.getState())) { + return; + } + return new Promise((resolve) => { + const unsub = store.subscribe(() => { + if (predicate(store.getState())) { + unsub(); + resolve(); + } + }); + }); +}