aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorLibravatar Kristóf Marussy <kristof@marussy.com>2022-01-25 17:56:28 +0100
committerLibravatar Kristóf Marussy <kristof@marussy.com>2022-02-08 21:43:17 +0100
commitc5f213df7b0d207667692395738c92c01f7e0837 (patch)
tree9a1220f67ea71df13bd01482eff0f0dc4c8a8c8a
parentrefactor: Store services in a map (diff)
downloadsophie-c5f213df7b0d207667692395738c92c01f7e0837.tar.gz
sophie-c5f213df7b0d207667692395738c92c01f7e0837.tar.zst
sophie-c5f213df7b0d207667692395738c92c01f7e0837.zip
refactor: Apply shared store patches in batches
Makes sure that the renderer always sees a consistent state. Signed-off-by: Kristóf Marussy <kristof@marussy.com>
-rw-r--r--packages/main/src/index.ts18
-rw-r--r--packages/main/src/stores/SharedStore.ts63
-rw-r--r--packages/preload/src/contextBridge/__tests__/createSophieRenderer.spec.ts16
-rw-r--r--packages/preload/src/contextBridge/createSophieRenderer.ts2
-rw-r--r--packages/renderer/src/stores/RendererStore.ts8
-rw-r--r--packages/shared/src/stores/SharedStore.ts2
6 files changed, 51 insertions, 58 deletions
diff --git a/packages/main/src/index.ts b/packages/main/src/index.ts
index 0978cd9..bcdc3d7 100644
--- a/packages/main/src/index.ts
+++ b/packages/main/src/index.ts
@@ -36,7 +36,7 @@ import {
36import { app, BrowserView, BrowserWindow, ipcMain } from 'electron'; 36import { app, BrowserView, BrowserWindow, ipcMain } from 'electron';
37import { ensureDirSync, readFile, readFileSync } from 'fs-extra'; 37import { ensureDirSync, readFile, readFileSync } from 'fs-extra';
38import { autorun } from 'mobx'; 38import { autorun } from 'mobx';
39import { getSnapshot, onPatch } from 'mobx-state-tree'; 39import { getSnapshot, onAction, onPatch } from 'mobx-state-tree';
40import osName from 'os-name'; 40import osName from 'os-name';
41 41
42import { 42import {
@@ -292,9 +292,23 @@ async function createWindow(): Promise<unknown> {
292 } 292 }
293 }); 293 });
294 294
295 const batchedPatches: unknown[] = [];
295 onPatch(store.shared, (patch) => { 296 onPatch(store.shared, (patch) => {
296 webContents.send(MainToRendererIpcMessage.SharedStorePatch, patch); 297 batchedPatches.push(patch);
297 }); 298 });
299 onAction(
300 store,
301 () => {
302 if (batchedPatches.length > 0) {
303 webContents.send(
304 MainToRendererIpcMessage.SharedStorePatch,
305 batchedPatches,
306 );
307 batchedPatches.splice(0);
308 }
309 },
310 true,
311 );
298 312
299 ipcMain.handle(ServiceToMainIpcMessage.ApiExposedInMainWorld, (event) => { 313 ipcMain.handle(ServiceToMainIpcMessage.ApiExposedInMainWorld, (event) => {
300 if (event.sender.id !== browserView.webContents.id) { 314 if (event.sender.id !== browserView.webContents.id) {
diff --git a/packages/main/src/stores/SharedStore.ts b/packages/main/src/stores/SharedStore.ts
index 9ec7963..499d1ee 100644
--- a/packages/main/src/stores/SharedStore.ts
+++ b/packages/main/src/stores/SharedStore.ts
@@ -55,25 +55,20 @@ function getConfigs<T>(models: { config: T }[]): T[] | undefined {
55 return models.length === 0 ? undefined : models.map((model) => model.config); 55 return models.length === 0 ? undefined : models.map((model) => model.config);
56} 56}
57 57
58type TypeWithSettings<C> = IType< 58function applySettings<
59 { id: string; settings: C }, 59 C,
60 unknown, 60 D extends IType<
61 { settings: IStateTreeNode<IType<C, unknown, unknown>> } 61 { id: string; settings: C },
62>; 62 unknown,
63 63 { settings: IStateTreeNode<IType<C, unknown, unknown>> }
64function deleteStaleModels<C, D extends TypeWithSettings<C>>( 64 >,
65>(
66 current: IMSTArray<IReferenceType<D>>,
65 currentById: IMSTMap<D>, 67 currentById: IMSTMap<D>,
66 toApplyById: Map<string, C>, 68 toApply: [string, C][],
67): void { 69): void {
70 const toApplyById = new Map(toApply);
68 const toDelete = new Set(currentById.keys()); 71 const toDelete = new Set(currentById.keys());
69 toApplyById.forEach((_settings, id) => toDelete.delete(id));
70 toDelete.forEach((id) => currentById.delete(id));
71}
72
73function applySettings<C, D extends TypeWithSettings<C>>(
74 currentById: IMSTMap<D>,
75 toApplyById: Map<string, C>,
76): void {
77 toApplyById.forEach((settingsSnapshot, id) => { 72 toApplyById.forEach((settingsSnapshot, id) => {
78 const model = currentById.get(id); 73 const model = currentById.get(id);
79 if (model === undefined) { 74 if (model === undefined) {
@@ -82,16 +77,13 @@ function applySettings<C, D extends TypeWithSettings<C>>(
82 settings: settingsSnapshot, 77 settings: settingsSnapshot,
83 }); 78 });
84 } else { 79 } else {
80 toDelete.delete(id);
85 applySnapshot(model.settings, settingsSnapshot); 81 applySnapshot(model.settings, settingsSnapshot);
86 } 82 }
87 }); 83 });
88} 84 current.clear();
89 85 toDelete.forEach((id) => currentById.delete(id));
90function pushReferences<C, D extends TypeWithSettings<C>>( 86 current.push(...toApply.map(([id]) => id));
91 list: IMSTArray<IReferenceType<D>>,
92 toApply: [string, C][],
93): void {
94 list.push(...toApply.map(([id]) => id));
95} 87}
96 88
97export const sharedStore = overrideProps(originalSharedStore, { 89export const sharedStore = overrideProps(originalSharedStore, {
@@ -115,9 +107,6 @@ export const sharedStore = overrideProps(originalSharedStore, {
115 })) 107 }))
116 .actions((self) => ({ 108 .actions((self) => ({
117 loadConfig(config: Config): void { 109 loadConfig(config: Config): void {
118 // `onPatch` will send store changes piecemeal without any attention to
119 // transaction boundaries. We must make sure that any state communicated to the
120 // renderer process is actually valid.
121 const { 110 const {
122 profiles, 111 profiles,
123 profilesById, 112 profilesById,
@@ -137,27 +126,9 @@ export const sharedStore = overrideProps(originalSharedStore, {
137 servicesConfig, 126 servicesConfig,
138 profilesToApply, 127 profilesToApply,
139 ); 128 );
140 const profilesToApplyById = new Map(profilesToApply); 129 applySettings(profiles, profilesById, profilesToApply);
141 const servicesToApplyById = new Map(servicesToApply); 130 applySettings(services, servicesById, servicesToApply);
142 // First remove any references to profiles and services that might be deleted.
143 self.selectedService = undefined;
144 services.clear();
145 profiles.clear();
146 // Delete all services that may depend on profiles that will be delete.
147 deleteStaleModels(servicesById, servicesToApplyById);
148 // Update existing profiles and add new profiles.
149 applySettings(profilesById, profilesToApplyById);
150 // Update existing services and add new services. This will make sure that no service
151 // depends on a profile that will be deleted.
152 applySettings(servicesById, servicesToApplyById);
153 // Now it's safe to delete stale profiles.
154 deleteStaleModels(profilesById, profilesToApplyById);
155 // We are ready to build new profile and service lists from the new models.
156 pushReferences(profiles, profilesToApply);
157 pushReferences(services, servicesToApply);
158 // Global settings may refer to particular profiles or services.
159 applySnapshot(settings, settingsToApply); 131 applySnapshot(settings, settingsToApply);
160 // Restore service selection (if applicable).
161 let newSelectedService; 132 let newSelectedService;
162 if (selectedServiceId !== undefined) { 133 if (selectedServiceId !== undefined) {
163 newSelectedService = servicesById.get(selectedServiceId); 134 newSelectedService = servicesById.get(selectedServiceId);
diff --git a/packages/preload/src/contextBridge/__tests__/createSophieRenderer.spec.ts b/packages/preload/src/contextBridge/__tests__/createSophieRenderer.spec.ts
index 2652c4e..4411789 100644
--- a/packages/preload/src/contextBridge/__tests__/createSophieRenderer.spec.ts
+++ b/packages/preload/src/contextBridge/__tests__/createSophieRenderer.spec.ts
@@ -51,11 +51,13 @@ const snapshot: SharedStoreSnapshotIn = {
51 shouldUseDarkColors: true, 51 shouldUseDarkColors: true,
52}; 52};
53 53
54const patch: IJsonPatch = { 54const patch: IJsonPatch[] = [
55 op: 'replace', 55 {
56 path: 'foo', 56 op: 'replace',
57 value: 'bar', 57 path: 'foo',
58}; 58 value: 'bar',
59 },
60];
59 61
60const action: Action = { 62const action: Action = {
61 action: 'set-theme-source', 63 action: 'set-theme-source',
@@ -90,7 +92,7 @@ describe('SharedStoreConnector', () => {
90 // eslint-disable-next-line @typescript-eslint/no-unused-vars, no-unused-vars 92 // eslint-disable-next-line @typescript-eslint/no-unused-vars, no-unused-vars
91 onSnapshot: jest.fn((_snapshot: SharedStoreSnapshotIn) => {}), 93 onSnapshot: jest.fn((_snapshot: SharedStoreSnapshotIn) => {}),
92 // eslint-disable-next-line @typescript-eslint/no-unused-vars, no-unused-vars 94 // eslint-disable-next-line @typescript-eslint/no-unused-vars, no-unused-vars
93 onPatch: jest.fn((_patch: IJsonPatch) => {}), 95 onPatch: jest.fn((_patch: IJsonPatch[]) => {}),
94 }; 96 };
95 97
96 beforeEach(() => { 98 beforeEach(() => {
@@ -234,7 +236,7 @@ describe('SharedStoreConnector', () => {
234 // eslint-disable-next-line @typescript-eslint/no-unused-vars, no-unused-vars 236 // eslint-disable-next-line @typescript-eslint/no-unused-vars, no-unused-vars
235 onSnapshot: jest.fn((_snapshot: SharedStoreSnapshotIn) => {}), 237 onSnapshot: jest.fn((_snapshot: SharedStoreSnapshotIn) => {}),
236 // eslint-disable-next-line @typescript-eslint/no-unused-vars, no-unused-vars 238 // eslint-disable-next-line @typescript-eslint/no-unused-vars, no-unused-vars
237 onPatch: jest.fn((_patch: IJsonPatch) => {}), 239 onPatch: jest.fn((_patch: IJsonPatch[]) => {}),
238 }; 240 };
239 241
240 it('should fetch a second snapshot', async () => { 242 it('should fetch a second snapshot', async () => {
diff --git a/packages/preload/src/contextBridge/createSophieRenderer.ts b/packages/preload/src/contextBridge/createSophieRenderer.ts
index 41accfd..6003c8b 100644
--- a/packages/preload/src/contextBridge/createSophieRenderer.ts
+++ b/packages/preload/src/contextBridge/createSophieRenderer.ts
@@ -42,7 +42,7 @@ class SharedStoreConnector {
42 (_event, patch) => { 42 (_event, patch) => {
43 try { 43 try {
44 // `mobx-state-tree` will validate the patch, so we can safely cast here. 44 // `mobx-state-tree` will validate the patch, so we can safely cast here.
45 this.listener?.onPatch(patch as IJsonPatch); 45 this.listener?.onPatch(patch as IJsonPatch[]);
46 } catch (error) { 46 } catch (error) {
47 log.error('Shared store listener onPatch failed', error); 47 log.error('Shared store listener onPatch failed', error);
48 this.listener = undefined; 48 this.listener = undefined;
diff --git a/packages/renderer/src/stores/RendererStore.ts b/packages/renderer/src/stores/RendererStore.ts
index a4e6197..4cbf6aa 100644
--- a/packages/renderer/src/stores/RendererStore.ts
+++ b/packages/renderer/src/stores/RendererStore.ts
@@ -25,7 +25,13 @@ import {
25 SophieRenderer, 25 SophieRenderer,
26 ThemeSource, 26 ThemeSource,
27} from '@sophie/shared'; 27} from '@sophie/shared';
28import { applySnapshot, applyPatch, Instance, types } from 'mobx-state-tree'; 28import {
29 applySnapshot,
30 applyPatch,
31 Instance,
32 types,
33 IJsonPatch,
34} from 'mobx-state-tree';
29 35
30import RendererEnv from '../env/RendererEnv'; 36import RendererEnv from '../env/RendererEnv';
31import getEnv from '../env/getEnv'; 37import getEnv from '../env/getEnv';
diff --git a/packages/shared/src/stores/SharedStore.ts b/packages/shared/src/stores/SharedStore.ts
index fc8372e..f301b9d 100644
--- a/packages/shared/src/stores/SharedStore.ts
+++ b/packages/shared/src/stores/SharedStore.ts
@@ -50,5 +50,5 @@ export interface SharedStoreSnapshotOut
50export interface SharedStoreListener { 50export interface SharedStoreListener {
51 onSnapshot(snapshot: SharedStoreSnapshotIn): void; 51 onSnapshot(snapshot: SharedStoreSnapshotIn): void;
52 52
53 onPatch(patch: IJsonPatch): void; 53 onPatch(patches: IJsonPatch[]): void;
54} 54}