diff options
author | Kristóf Marussy <kristof@marussy.com> | 2022-01-25 17:56:28 +0100 |
---|---|---|
committer | Kristóf Marussy <kristof@marussy.com> | 2022-02-08 21:43:17 +0100 |
commit | c5f213df7b0d207667692395738c92c01f7e0837 (patch) | |
tree | 9a1220f67ea71df13bd01482eff0f0dc4c8a8c8a /packages | |
parent | refactor: Store services in a map (diff) | |
download | sophie-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>
Diffstat (limited to 'packages')
-rw-r--r-- | packages/main/src/index.ts | 18 | ||||
-rw-r--r-- | packages/main/src/stores/SharedStore.ts | 63 | ||||
-rw-r--r-- | packages/preload/src/contextBridge/__tests__/createSophieRenderer.spec.ts | 16 | ||||
-rw-r--r-- | packages/preload/src/contextBridge/createSophieRenderer.ts | 2 | ||||
-rw-r--r-- | packages/renderer/src/stores/RendererStore.ts | 8 | ||||
-rw-r--r-- | packages/shared/src/stores/SharedStore.ts | 2 |
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 { | |||
36 | import { app, BrowserView, BrowserWindow, ipcMain } from 'electron'; | 36 | import { app, BrowserView, BrowserWindow, ipcMain } from 'electron'; |
37 | import { ensureDirSync, readFile, readFileSync } from 'fs-extra'; | 37 | import { ensureDirSync, readFile, readFileSync } from 'fs-extra'; |
38 | import { autorun } from 'mobx'; | 38 | import { autorun } from 'mobx'; |
39 | import { getSnapshot, onPatch } from 'mobx-state-tree'; | 39 | import { getSnapshot, onAction, onPatch } from 'mobx-state-tree'; |
40 | import osName from 'os-name'; | 40 | import osName from 'os-name'; |
41 | 41 | ||
42 | import { | 42 | import { |
@@ -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 | ||
58 | type TypeWithSettings<C> = IType< | 58 | function 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>> } | |
64 | function 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 | |||
73 | function 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)); | |
90 | function 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 | ||
97 | export const sharedStore = overrideProps(originalSharedStore, { | 89 | export 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 | ||
54 | const patch: IJsonPatch = { | 54 | const 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 | ||
60 | const action: Action = { | 62 | const 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'; |
28 | import { applySnapshot, applyPatch, Instance, types } from 'mobx-state-tree'; | 28 | import { |
29 | applySnapshot, | ||
30 | applyPatch, | ||
31 | Instance, | ||
32 | types, | ||
33 | IJsonPatch, | ||
34 | } from 'mobx-state-tree'; | ||
29 | 35 | ||
30 | import RendererEnv from '../env/RendererEnv'; | 36 | import RendererEnv from '../env/RendererEnv'; |
31 | import getEnv from '../env/getEnv'; | 37 | import 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 | |||
50 | export interface SharedStoreListener { | 50 | export 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 | } |