Fixed re-enabling a still selected plugin

Summary: While testing manually discovered the sandy plugin infra din't cover the case that a plugin can be selected but not enabled at the same time. Added test and fixed that. Reviewed By: nikoant Differential Revision: D22308597 fbshipit-source-id: 6cef2b543013ee81cee449396d523dd9a657ad1c
2020-07-01 08:58:40 -07:00
parent d16c6061c1
commit 581ddafd18
6 changed files with 69 additions and 31 deletions
--- a/desktop/app/src/tests/PluginContainer.node.tsx
+++ b/desktop/app/src/tests/PluginContainer.node.tsx
@@ -7,18 +7,17 @@
 * @format
 */

-import React, {useContext} from 'react';
+import React from 'react';
 import produce from 'immer';
 import {FlipperPlugin} from '../plugin';
 import {renderMockFlipperWithPlugin} from '../test-utils/createMockFlipperWithPlugin';
 import {
-  SandyPluginContext,
  SandyPluginDefinition,
  FlipperClient,
  TestUtils,
  usePlugin,
 } from 'flipper-plugin';
-import {selectPlugin} from '../reducers/connections';
+import {selectPlugin, starPlugin} from '../reducers/connections';

 interface PersistedState {
  count: 1;
@@ -132,6 +131,9 @@ test('PluginContainer can render Sandy plugins', async () => {
    client,
    store,
  } = await renderMockFlipperWithPlugin(definition);
+
+  expect(client.rawSend).toBeCalledWith('init', {plugin: 'TestPlugin'});
+
  expect(renderer.baseElement).toMatchInlineSnapshot(`
        <body>
          <div>
@@ -164,8 +166,6 @@ test('PluginContainer can render Sandy plugins', async () => {
  expect(pluginInstance.connectedStub).toBeCalledTimes(1);
  expect(pluginInstance.disconnectedStub).toBeCalledTimes(0);

-  // TODO: check that messages have arrived T68683442
-
  // select non existing plugin
  act(() => {
    store.dispatch(
@@ -176,6 +176,8 @@ test('PluginContainer can render Sandy plugins', async () => {
    );
  });

+  expect(client.rawSend).toBeCalledWith('deinit', {plugin: 'TestPlugin'});
+
  expect(renderer.baseElement).toMatchInlineSnapshot(`
    <body>
      <div />
@@ -195,4 +197,36 @@ test('PluginContainer can render Sandy plugins', async () => {
  });
  expect(pluginInstance.connectedStub).toBeCalledTimes(2);
  expect(pluginInstance.disconnectedStub).toBeCalledTimes(1);
+  expect(client.rawSend).toBeCalledWith('init', {plugin: 'TestPlugin'});
+
+  // disable
+  act(() => {
+    store.dispatch(
+      starPlugin({
+        plugin: definition,
+        selectedApp: client.query.app,
+      }),
+    );
+  });
+  expect(pluginInstance.connectedStub).toBeCalledTimes(2);
+  expect(pluginInstance.disconnectedStub).toBeCalledTimes(2);
+  expect(client.rawSend).toBeCalledWith('deinit', {plugin: 'TestPlugin'});
+
+  // re-enable
+  act(() => {
+    store.dispatch(
+      starPlugin({
+        plugin: definition,
+        selectedApp: client.query.app,
+      }),
+    );
+  });
+  // note: this is the old pluginInstance, so that one is not reconnected!
+  expect(pluginInstance.connectedStub).toBeCalledTimes(2);
+  expect(pluginInstance.disconnectedStub).toBeCalledTimes(2);
+
+  expect(
+    client.sandyPluginStates.get('TestPlugin')!.instanceApi.connectedStub,
+  ).toBeCalledTimes(1);
+  expect(client.rawSend).toBeCalledWith('init', {plugin: 'TestPlugin'});
 });
--- a/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx
+++ b/desktop/app/src/test-utils/createMockFlipperWithPlugin.tsx
@@ -137,6 +137,7 @@ export async function createMockFlipperWithPlugin(
          );
      }
    };
+    client.rawSend = jest.fn();

    // enable the plugin
    if (
--- a/desktop/flipper-plugin/src/index.ts
+++ b/desktop/flipper-plugin/src/index.ts
@@ -13,7 +13,7 @@ export {SandyPluginInstance, FlipperClient} from './plugin/Plugin';
 export {SandyPluginDefinition} from './plugin/SandyPluginDefinition';
 export {SandyPluginRenderer} from './plugin/PluginRenderer';
 export {SandyPluginContext, usePlugin} from './plugin/PluginContext';
-export {createState as createValue, useValue, Atom} from './state/atom';
+export {createState, useValue, Atom} from './state/atom';

 // It's not ideal that this exists in flipper-plugin sources directly,
 // but is the least pain for plugin authors.
--- a/desktop/flipper-plugin/src/plugin/Plugin.tsx
+++ b/desktop/flipper-plugin/src/plugin/Plugin.tsx
@@ -149,7 +149,11 @@ export class SandyPluginInstance {

  // the plugin is deselected in the UI
  deactivate() {
-    this.assertNotDestroyed();
+    if (this.destroyed) {
+      // this can happen if the plugin is disabled while active in the UI.
+      // In that case deinit & destroy is already triggered from the STAR_PLUGIN action
+      return;
+    }
    const pluginId = this.definition.id;
    if (!this.realClient.isBackgroundPlugin(pluginId)) {
      this.realClient.deinitPlugin(pluginId);
@@ -174,7 +178,9 @@ export class SandyPluginInstance {

  destroy() {
    this.assertNotDestroyed();
-    this.disconnect();
+    if (this.connected) {
+      this.realClient.deinitPlugin(this.definition.id);
+    }
    this.events.emit('destroy');
    this.destroyed = true;
  }
--- a/desktop/flipper-plugin/src/plugin/PluginRenderer.tsx
+++ b/desktop/flipper-plugin/src/plugin/PluginRenderer.tsx
@@ -18,24 +18,17 @@ type Props = {
 /**
 * Component to render a Sandy plugin container
 */
-export const SandyPluginRenderer = memo(
-  ({plugin}: Props) => {
-    useEffect(() => {
-      plugin.activate();
-      return () => {
-        plugin.deactivate();
-      };
-    }, [plugin]);
+export const SandyPluginRenderer = memo(({plugin}: Props) => {
+  useEffect(() => {
+    plugin.activate();
+    return () => {
+      plugin.deactivate();
+    };
+  }, [plugin]);

-    return (
-      <SandyPluginContext.Provider value={plugin}>
-        {createElement(plugin.definition.module.Component)}
-      </SandyPluginContext.Provider>
-    );
-  },
-  () => {
-    // One of the goals of the ModernPluginContainer is that we want to prevent it from rendering
-    // for any outside change. Whatever happens outside of us, we don't care. If it is relevant for use, we take care about it from the insde
-    return true;
-  },
-);
+  return (
+    <SandyPluginContext.Provider value={plugin}>
+      {createElement(plugin.definition.module.Component)}
+    </SandyPluginContext.Provider>
+  );
+});
--- a/desktop/flipper-plugin/src/test-utils/test-utils.tsx
+++ b/desktop/flipper-plugin/src/test-utils/test-utils.tsx
@@ -117,8 +117,12 @@ export function startPlugin<Module extends FlipperPluginModule<any>>(
      // as from testing perspective the difference shouldn't matter
      return false;
    },
-    initPlugin(_pluginId: string) {},
-    deinitPlugin(_pluginId: string) {},
+    initPlugin(_pluginId: string) {
+      pluginInstance.connect();
+    },
+    deinitPlugin(_pluginId: string) {
+      pluginInstance.disconnect();
+    },
    call(
      api: string,
      method: string,
@@ -131,7 +135,7 @@ export function startPlugin<Module extends FlipperPluginModule<any>>(

  const pluginInstance = new SandyPluginInstance(fakeFlipper, definition);
  // we start connected
-  pluginInstance.connect();
+  pluginInstance.activate();

  const res: StartPluginResult<Module> = {
    module,