From 0e4a6d659b93e0b5c2b47e25349fdde0f0415dc9 Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Tue, 14 Jul 2020 09:04:59 -0700 Subject: [PATCH] Make sure plugins can serialize and deserialize Summary: This plugin adds serialization capabilities to Sandy plugins buy setting the a `persist: ` flag. This shouldn't be used for state that is unserializable, too big, too sensitive, or irrelevant during export / import. Using an explicit `persist` flag is done to make plugins robust to changes over time; as long as the key is kept the same, state variables can be renamed and reordered without breaking the import / export format. Also it allows us to detect some changes in the import / export format and warn about it. Alternative designs considered but not implemented would be: 1. requiring the user to explicitly return the state from the factory (e.g. `const todos = createState([]); return { todos }`, 2. or construct the state from client (e.g. `const todos = client.createState([])`) 3. enable persistence by default, and store states in the order the states were created (much like useState in React). This was implemented in the first versions of this diff, but as pointed out in the discussions, this is too sensitive too (accidental) format changes, as the storage format would be quite implicit A nice benefit of the current approach, especially compared with alternative approach 1, is that state being restored is immediately visible in the plugin factory. In other words, directly after initialization `const todos = createState([])`, the `todos.get()` is actually set to the state that is being restored, rather than having still the initial state which is only overridden rather. So this behaves very much like the `useState` hook in React. Furthermore, in the future we could use the same `persist` key in combination with other options, such as `saveToLocalStorage`, in case some state acts as a 'preference' (T69989583). `TestUtils.startPlugin` supports starting plugins with an initial state by using the optional `initialState` field Actually wiring up the serialization and deserialization into the export / import functionality of Flipper is done in the next diff. Reviewed By: jknoxville Differential Revision: D22432770 fbshipit-source-id: 9a4849582c2f6f54d1e40f65a6cba73092c28fe8 --- .../__tests__/messageQueueSandy.node.tsx | 66 ++++++++-------- .../src/__tests__/TestPlugin.tsx | 11 ++- .../src/__tests__/test-utils.node.tsx | 75 ++++++++++++++++++- desktop/flipper-plugin/src/plugin/Plugin.tsx | 27 ++++++- desktop/flipper-plugin/src/state/atom.tsx | 39 +++++++++- .../src/test-utils/test-utils.tsx | 14 +++- desktop/tsconfig.base.json | 25 ++----- 7 files changed, 192 insertions(+), 65 deletions(-) diff --git a/desktop/app/src/utils/__tests__/messageQueueSandy.node.tsx b/desktop/app/src/utils/__tests__/messageQueueSandy.node.tsx index 002a492e4..1a33b87b1 100644 --- a/desktop/app/src/utils/__tests__/messageQueueSandy.node.tsx +++ b/desktop/app/src/utils/__tests__/messageQueueSandy.node.tsx @@ -514,40 +514,40 @@ test('client - incoming messages are buffered and flushed together', async () => } `); expect(client.messageBuffer).toMatchInlineSnapshot(` - Object { - "TestApp#Android#MockAndroidDevice#serial#DevicePlugin": Object { - "messages": Array [ - Object { - "api": "DevicePlugin", - "method": "log", - "params": Object { - "line": "suff", - }, - }, - ], - "plugin": [Function], + Object { + "TestApp#Android#MockAndroidDevice#serial#DevicePlugin": Object { + "messages": Array [ + Object { + "api": "DevicePlugin", + "method": "log", + "params": Object { + "line": "suff", + }, }, - "TestApp#Android#MockAndroidDevice#serial#TestPlugin": Object { - "messages": Array [ - Object { - "api": "TestPlugin", - "method": "inc", - "params": Object { - "delta": 2, - }, - }, - Object { - "api": "TestPlugin", - "method": "inc", - "params": Object { - "delta": 3, - }, - }, - ], - "plugin": undefined, + ], + "plugin": [Function], + }, + "TestApp#Android#MockAndroidDevice#serial#TestPlugin": Object { + "messages": Array [ + Object { + "api": "TestPlugin", + "method": "inc", + "params": Object { + "delta": 2, + }, }, - } - `); + Object { + "api": "TestPlugin", + "method": "inc", + "params": Object { + "delta": 3, + }, + }, + ], + "plugin": "[SandyPluginInstance]", + }, + } + `); expect(client.messageBuffer[pluginKey].plugin).toBeInstanceOf( SandyPluginInstance, ); @@ -661,7 +661,7 @@ test('queue - messages that have not yet flushed be lost when disabling the plug }, }, ], - "plugin": undefined, + "plugin": "[SandyPluginInstance]", }, } `); diff --git a/desktop/flipper-plugin/src/__tests__/TestPlugin.tsx b/desktop/flipper-plugin/src/__tests__/TestPlugin.tsx index 41a601c43..db67005b1 100644 --- a/desktop/flipper-plugin/src/__tests__/TestPlugin.tsx +++ b/desktop/flipper-plugin/src/__tests__/TestPlugin.tsx @@ -26,9 +26,14 @@ export function plugin(client: FlipperClient) { const connectStub = jest.fn(); const disconnectStub = jest.fn(); const destroyStub = jest.fn(); - const state = createState({ - count: 0, - }); + const state = createState( + { + count: 0, + }, + { + persist: 'counter', + }, + ); // TODO: add tests for sending and receiving data T68683442 // including typescript assertions diff --git a/desktop/flipper-plugin/src/__tests__/test-utils.node.tsx b/desktop/flipper-plugin/src/__tests__/test-utils.node.tsx index b7d62ac46..cf8eeccac 100644 --- a/desktop/flipper-plugin/src/__tests__/test-utils.node.tsx +++ b/desktop/flipper-plugin/src/__tests__/test-utils.node.tsx @@ -9,6 +9,7 @@ import * as TestUtils from '../test-utils/test-utils'; import * as testPlugin from './TestPlugin'; +import {createState} from '../state/atom'; test('it can start a plugin and lifecycle events', () => { const {instance, ...p} = TestUtils.startPlugin(testPlugin); @@ -114,9 +115,81 @@ test('a plugin cannot send messages after being disconnected', async () => { }); test('a plugin can receive messages', async () => { - const {instance, sendEvent} = TestUtils.startPlugin(testPlugin); + const {instance, sendEvent, exportState} = TestUtils.startPlugin(testPlugin); expect(instance.state.get().count).toBe(0); sendEvent('inc', {delta: 2}); expect(instance.state.get().count).toBe(2); + expect(exportState()).toMatchInlineSnapshot(` + Object { + "counter": Object { + "count": 2, + }, + } + `); +}); + +test('plugins support non-serializable state', async () => { + const {exportState} = TestUtils.startPlugin({ + plugin() { + const field1 = createState(true); + const field2 = createState( + { + test: 3, + }, + { + persist: 'field2', + }, + ); + return { + field1, + field2, + }; + }, + Component() { + return null; + }, + }); + // states are serialized in creation order + expect(exportState()).toEqual({field2: {test: 3}}); +}); + +test('plugins support restoring state', async () => { + const {exportState} = TestUtils.startPlugin( + { + plugin() { + const field1 = createState(1, {persist: 'field1'}); + const field2 = createState(2); + const field3 = createState(3, {persist: 'field3'}); + expect(field1.get()).toBe('a'); + expect(field2.get()).toBe(2); + expect(field3.get()).toBe('b'); + return {}; + }, + Component() { + return null; + }, + }, + { + initialState: {field1: 'a', field3: 'b'}, + }, + ); + expect(exportState()).toEqual({field1: 'a', field3: 'b'}); +}); + +test('plugins cannot use a persist key twice', async () => { + expect(() => { + TestUtils.startPlugin({ + plugin() { + const field1 = createState(1, {persist: 'test'}); + const field2 = createState(2, {persist: 'test'}); + return {field1, field2}; + }, + Component() { + return null; + }, + }); + }).toThrowErrorMatchingInlineSnapshot( + `"Some other state is already persisting with key \\"test\\""`, + ); }); diff --git a/desktop/flipper-plugin/src/plugin/Plugin.tsx b/desktop/flipper-plugin/src/plugin/Plugin.tsx index b71a6bad3..945b2aa38 100644 --- a/desktop/flipper-plugin/src/plugin/Plugin.tsx +++ b/desktop/flipper-plugin/src/plugin/Plugin.tsx @@ -9,6 +9,7 @@ import {SandyPluginDefinition} from './SandyPluginDefinition'; import {EventEmitter} from 'events'; +import {Atom} from '../state/atom'; type EventsContract = Record; type MethodsContract = Record Promise>; @@ -88,6 +89,8 @@ export type FlipperPluginFactory< export type FlipperPluginComponent = React.FC<{}>; +export let currentPluginInstance: SandyPluginInstance | undefined = undefined; + export class SandyPluginInstance { static is(thing: any): thing is SandyPluginInstance { return thing instanceof SandyPluginInstance; @@ -106,9 +109,15 @@ export class SandyPluginInstance { destroyed = false; events = new EventEmitter(); + // temporarily field that is used during deserialization + initialStates?: Record; + // all the atoms that should be serialized when making an export / import + rootStates: Record> = {}; + constructor( realClient: RealFlipperClient, definition: SandyPluginDefinition, + initialStates?: Record, ) { this.realClient = realClient; this.definition = definition; @@ -135,7 +144,14 @@ export class SandyPluginInstance { this.events.on('event-' + event, callback); }, }; - this.instanceApi = definition.module.plugin(this.client); + currentPluginInstance = this; + this.initialStates = initialStates; + try { + this.instanceApi = definition.module.plugin(this.client); + } finally { + this.initialStates = undefined; + currentPluginInstance = undefined; + } } // the plugin is selected in the UI @@ -192,8 +208,13 @@ export class SandyPluginInstance { } toJSON() { - this.assertNotDestroyed(); - // TODO: T68683449 + return '[SandyPluginInstance]'; + } + + exportState() { + return Object.fromEntries( + Object.entries(this.rootStates).map(([key, atom]) => [key, atom.get()]), + ); } private assertNotDestroyed() { diff --git a/desktop/flipper-plugin/src/state/atom.tsx b/desktop/flipper-plugin/src/state/atom.tsx index 15a64b09a..4cdc9fb32 100644 --- a/desktop/flipper-plugin/src/state/atom.tsx +++ b/desktop/flipper-plugin/src/state/atom.tsx @@ -9,6 +9,7 @@ import {produce} from 'immer'; import {useState, useEffect} from 'react'; +import {currentPluginInstance} from '../plugin/Plugin'; export type Atom = { get(): T; @@ -56,8 +57,38 @@ class AtomValue implements Atom { } } -export function createState(initialValue: T): Atom { - return new AtomValue(initialValue); +type StateOptions = { + /** + * Should this state persist when exporting a plugin? + * If set, the atom will be saved / loaded under the key provided + */ + persist?: string; +}; + +export function createState( + initialValue: T, + options: StateOptions = {}, +): Atom { + const atom = new AtomValue(initialValue); + if (currentPluginInstance && options.persist) { + const {initialStates, rootStates} = currentPluginInstance; + if (initialStates) { + if (options.persist in initialStates) { + atom.set(initialStates[options.persist]); + } else { + console.warn( + `Tried to initialize plugin with existing data, however data for "${options.persist}" is missing. Was the export created with a different Flipper version?`, + ); + } + } + if (rootStates[options.persist]) { + throw new Error( + `Some other state is already persisting with key "${options.persist}"`, + ); + } + rootStates[options.persist] = atom; + } + return atom; } export function useValue(atom: Atom): T { @@ -73,3 +104,7 @@ export function useValue(atom: Atom): T { }, [atom]); return localValue; } + +export function isAtom(value: any): value is Atom { + return value instanceof AtomValue; +} diff --git a/desktop/flipper-plugin/src/test-utils/test-utils.tsx b/desktop/flipper-plugin/src/test-utils/test-utils.tsx index 51b13afc0..980898fee 100644 --- a/desktop/flipper-plugin/src/test-utils/test-utils.tsx +++ b/desktop/flipper-plugin/src/test-utils/test-utils.tsx @@ -31,8 +31,7 @@ import {act} from '@testing-library/react'; type Renderer = RenderResult; interface StartPluginOptions { - // TODO: support initial events T68683442 (and type correctly) - // TODO: support initial state T68683449 (and type correctly) + initialState?: Record; } type ExtractClientType> = Parameters< @@ -100,11 +99,13 @@ interface StartPluginResult> { params: any; // afaik we can't type this :-( }[], ): void; + + exportState(): any; } export function startPlugin>( module: Module, - _options?: StartPluginOptions, + options?: StartPluginOptions, ): StartPluginResult { const definition = new SandyPluginDefinition( createMockPluginDetails(), @@ -134,7 +135,11 @@ export function startPlugin>( }, }; - const pluginInstance = new SandyPluginInstance(fakeFlipper, definition); + const pluginInstance = new SandyPluginInstance( + fakeFlipper, + definition, + options?.initialState, + ); // we start connected pluginInstance.activate(); @@ -158,6 +163,7 @@ export function startPlugin>( pluginInstance.receiveMessages(messages as any); }); }, + exportState: () => pluginInstance.exportState(), }; // @ts-ignore res._backingInstance = pluginInstance; diff --git a/desktop/tsconfig.base.json b/desktop/tsconfig.base.json index acafe847c..fd70f3f64 100644 --- a/desktop/tsconfig.base.json +++ b/desktop/tsconfig.base.json @@ -1,13 +1,9 @@ { "compilerOptions": { "module": "commonjs", - "lib": [ - "es7", - "dom", - "es2017" - ], + "lib": ["es7", "dom", "es2019"], "esModuleInterop": true, - "target": "ES2017", + "target": "ES2019", "removeComments": true, "preserveConstEnums": true, "sourceMap": true, @@ -22,19 +18,10 @@ "allowJs": true, "rootDir": ".", "paths": { - "flipper": [ - "./app/src" - ], - "flipper-plugin": [ - "./flipper-plugin/src" - ], - "flipper-*": [ - "./*/src" - ], - "*": [ - "./*", - "./types/*" - ] + "flipper": ["./app/src"], + "flipper-plugin": ["./flipper-plugin/src"], + "flipper-*": ["./*/src"], + "*": ["./*", "./types/*"] } } }