From eb096d07e276e1b4c8040220c47f186d02841e14 Mon Sep 17 00:00:00 2001 From: undergroundwires Date: Fri, 1 Sep 2023 18:14:25 +0200 Subject: [PATCH] Fix memory leaks via auto-unsubscribing and DI This commit simplifies event handling, providing a unified and robust way to handle event lifecycling. This way, it fixes events not being unsubscribed when state is changed. Introduce a new function in `EventSubscriptionCollection` to remove existing events and adding new events. This provides an easier to use API, which leads to code that's easier to understand. It also prevents potential bugs that may occur due to forgetting to call both functions. It fixes `TheScriptsMenu` not unregistering events on state change. Other improvements include: - Include a getter to get total amount of registered subcriptions. This helps in unit testing. - Have nullish checks to prevent potential errors further down the execution. - Use array instead of rest parameters to increase readability and simplify tests. Ensure `SliderHandler` stops resizes on unmount, unsubscribing from all events and resetting state to default. Update `injectionKeys` to do imports as types to avoid circular dependencies. Simplify importing `injectionKeys` to enable and strict typings for iterating injection keys. Add tests covering new behavior. --- docs/presentation.md | 4 +- .../Events/EventSubscriptionCollection.ts | 19 +- .../Events/IEventSubscriptionCollection.ts | 6 +- .../bootstrapping/DependencyProvider.ts | 41 ++-- .../Instructions/InstructionList.vue | 4 +- .../Code/CodeButtons/TheCodeButtons.vue | 22 +- .../components/Code/TheCodeArea.vue | 26 +-- .../Scripts/Menu/Selector/TheSelector.vue | 26 +-- .../components/Scripts/Menu/TheOsChanger.vue | 6 +- .../Scripts/Menu/TheScriptsMenu.vue | 38 ++-- .../Scripts/Slider/SliderHandle.vue | 6 +- .../Scripts/View/Cards/CardList.vue | 4 +- .../Scripts/View/Cards/CardListItem.vue | 20 +- .../Scripts/View/ScriptsTree/ScriptsTree.vue | 23 +- .../SelectableTree/Node/RevertToggle.vue | 18 +- .../Scripts/View/TheScriptsView.vue | 36 ++-- .../Shared/Hooks/UseAutoUnsubscribedEvents.ts | 19 ++ .../Shared/Hooks/UseCollectionState.ts | 24 ++- .../components/TheFooter/DownloadUrlList.vue | 4 +- .../TheFooter/DownloadUrlListItem.vue | 6 +- .../components/TheFooter/PrivacyPolicy.vue | 6 +- .../components/TheFooter/TheFooter.vue | 6 +- src/presentation/components/TheHeader.vue | 4 +- src/presentation/components/TheSearchBar.vue | 39 ++-- src/presentation/injectionSymbols.ts | 14 +- .../EventSubscriptionCollection.spec.ts | 179 ++++++++++++++-- .../bootstrapping/DependencyProvider.spec.ts | 99 +++++++++ .../Scripts/View/TheScriptsView.spec.ts | 9 +- .../Hooks/UseAutoUnsubscribedEvents.spec.ts | 68 ++++++ .../Shared/Hooks/UseCollectionState.spec.ts | 197 +++++++++++++++--- tests/unit/presentation/injectionSymbols.ts | 24 +++ .../Stubs/EventSubscriptionCollectionStub.ts | 42 +++- .../shared/Stubs/EventSubscriptionStub.ts | 20 +- .../Stubs/UseAutoUnsubscribedEventsStub.ts | 10 + .../Stubs/VueDependencyInjectionApiStub.ts | 14 ++ tests/unit/shared/TestCases/SingletonTests.ts | 16 +- tests/unit/shared/TestCases/TransientFacto | 0 37 files changed, 856 insertions(+), 243 deletions(-) create mode 100644 src/presentation/components/Shared/Hooks/UseAutoUnsubscribedEvents.ts create mode 100644 tests/unit/presentation/bootstrapping/DependencyProvider.spec.ts create mode 100644 tests/unit/presentation/components/Shared/Hooks/UseAutoUnsubscribedEvents.spec.ts create mode 100644 tests/unit/presentation/injectionSymbols.ts create mode 100644 tests/unit/shared/Stubs/UseAutoUnsubscribedEventsStub.ts create mode 100644 tests/unit/shared/Stubs/VueDependencyInjectionApiStub.ts create mode 100644 tests/unit/shared/TestCases/TransientFacto diff --git a/docs/presentation.md b/docs/presentation.md index 95f8303d..d2648b2e 100644 --- a/docs/presentation.md +++ b/docs/presentation.md @@ -14,12 +14,12 @@ The presentation layer uses an event-driven architecture for bidirectional react - [**`bootstrapping/`**](./../src/presentation/bootstrapping/): Registers Vue components and plugins. - [**`components/`**](./../src/presentation/components/): Contains Vue components and helpers. - [**`Shared/`**](./../src/presentation/components/Shared): Contains shared Vue components and helpers. - - [**`hooks`**](../src/presentation/components/Shared/Hooks): Hooks used by components through [dependency injection](#dependency-injections). + - [**`Hooks`**](../src/presentation/components/Shared/Hooks): Hooks used by components through [dependency injection](#dependency-injections). - [**`/public/`**](../src/presentation/public/): Contains static assets. - [**`assets/`**](./../src/presentation/assets/styles/): Contains assets processed by Vite. - [**`fonts/`**](./../src/presentation/assets/fonts/): Contains fonts. - [**`styles/`**](./../src/presentation/assets/styles/): Contains shared styles. - - [**`components/`**](./../src/presentation/assets/styles/components): Contains styles for Vue components. + - [**`components/`**](./../src/presentation/assets/styles/components): Contains styles coupled to Vue components. - [**`vendors-extensions/`**](./../src/presentation/assets/styles/third-party-extensions): Contains styles for third-party components. - [**`main.scss`**](./../src/presentation/assets/styles/main.scss): Main Sass file, imported by other components as single entrypoint. - [**`main.ts`**](./../src/presentation/main.ts): Starts Vue app. diff --git a/src/infrastructure/Events/EventSubscriptionCollection.ts b/src/infrastructure/Events/EventSubscriptionCollection.ts index f8ce7baf..df06163e 100644 --- a/src/infrastructure/Events/EventSubscriptionCollection.ts +++ b/src/infrastructure/Events/EventSubscriptionCollection.ts @@ -1,10 +1,20 @@ -import { IEventSubscription } from './IEventSource'; import { IEventSubscriptionCollection } from './IEventSubscriptionCollection'; +import { IEventSubscription } from './IEventSource'; export class EventSubscriptionCollection implements IEventSubscriptionCollection { private readonly subscriptions = new Array(); - public register(...subscriptions: IEventSubscription[]) { + public get subscriptionCount() { + return this.subscriptions.length; + } + + public register(subscriptions: IEventSubscription[]) { + if (!subscriptions || subscriptions.length === 0) { + throw new Error('missing subscriptions'); + } + if (subscriptions.some((subscription) => !subscription)) { + throw new Error('missing subscription in list'); + } this.subscriptions.push(...subscriptions); } @@ -12,4 +22,9 @@ export class EventSubscriptionCollection implements IEventSubscriptionCollection this.subscriptions.forEach((listener) => listener.unsubscribe()); this.subscriptions.splice(0, this.subscriptions.length); } + + public unsubscribeAllAndRegister(subscriptions: IEventSubscription[]) { + this.unsubscribeAll(); + this.register(subscriptions); + } } diff --git a/src/infrastructure/Events/IEventSubscriptionCollection.ts b/src/infrastructure/Events/IEventSubscriptionCollection.ts index 57d09cb0..a8b0c9ab 100644 --- a/src/infrastructure/Events/IEventSubscriptionCollection.ts +++ b/src/infrastructure/Events/IEventSubscriptionCollection.ts @@ -1,7 +1,9 @@ import { IEventSubscription } from '@/infrastructure/Events/IEventSource'; export interface IEventSubscriptionCollection { - register(...subscriptions: IEventSubscription[]); + readonly subscriptionCount: number; - unsubscribeAll(); + register(subscriptions: IEventSubscription[]): void; + unsubscribeAll(): void; + unsubscribeAllAndRegister(subscriptions: IEventSubscription[]); } diff --git a/src/presentation/bootstrapping/DependencyProvider.ts b/src/presentation/bootstrapping/DependencyProvider.ts index c08cdc25..01e9a50f 100644 --- a/src/presentation/bootstrapping/DependencyProvider.ts +++ b/src/presentation/bootstrapping/DependencyProvider.ts @@ -1,28 +1,31 @@ -import { InjectionKey, provide } from 'vue'; +import { InjectionKey, provide, inject } from 'vue'; import { useCollectionState } from '@/presentation/components/Shared/Hooks/UseCollectionState'; import { useApplication } from '@/presentation/components/Shared/Hooks/UseApplication'; -import { - useCollectionStateKey, useApplicationKey, useRuntimeEnvironmentKey, -} from '@/presentation/injectionSymbols'; +import { useAutoUnsubscribedEvents } from '@/presentation/components/Shared/Hooks/UseAutoUnsubscribedEvents'; import { IApplicationContext } from '@/application/Context/IApplicationContext'; import { RuntimeEnvironment } from '@/infrastructure/RuntimeEnvironment/RuntimeEnvironment'; +import { InjectionKeys } from '@/presentation/injectionSymbols'; -export function provideDependencies(context: IApplicationContext) { - registerSingleton(useApplicationKey, useApplication(context.app)); - registerTransient(useCollectionStateKey, () => useCollectionState(context)); - registerSingleton(useRuntimeEnvironmentKey, RuntimeEnvironment.CurrentEnvironment); -} - -function registerSingleton( - key: InjectionKey, - value: T, +export function provideDependencies( + context: IApplicationContext, + api: VueDependencyInjectionApi = { provide, inject }, ) { - provide(key, value); + const registerSingleton = (key: InjectionKey, value: T) => api.provide(key, value); + const registerTransient = ( + key: InjectionKey<() => T>, + factory: () => T, + ) => api.provide(key, factory); + + registerSingleton(InjectionKeys.useApplication, useApplication(context.app)); + registerSingleton(InjectionKeys.useRuntimeEnvironment, RuntimeEnvironment.CurrentEnvironment); + registerTransient(InjectionKeys.useAutoUnsubscribedEvents, () => useAutoUnsubscribedEvents()); + registerTransient(InjectionKeys.useCollectionState, () => { + const { events } = api.inject(InjectionKeys.useAutoUnsubscribedEvents)(); + return useCollectionState(context, events); + }); } -function registerTransient( - key: InjectionKey<() => T>, - factory: () => T, -) { - provide(key, factory); +export interface VueDependencyInjectionApi { + provide(key: InjectionKey, value: T): void; + inject(key: InjectionKey): T; } diff --git a/src/presentation/components/Code/CodeButtons/Instructions/InstructionList.vue b/src/presentation/components/Code/CodeButtons/Instructions/InstructionList.vue index 329fd6b7..acfc24ee 100644 --- a/src/presentation/components/Code/CodeButtons/Instructions/InstructionList.vue +++ b/src/presentation/components/Code/CodeButtons/Instructions/InstructionList.vue @@ -59,7 +59,7 @@ import { defineComponent, PropType, computed, inject, } from 'vue'; -import { useApplicationKey } from '@/presentation/injectionSymbols'; +import { InjectionKeys } from '@/presentation/injectionSymbols'; import { OperatingSystem } from '@/domain/OperatingSystem'; import TooltipWrapper from '@/presentation/components/Shared/TooltipWrapper.vue'; import CodeInstruction from './CodeInstruction.vue'; @@ -77,7 +77,7 @@ export default defineComponent({ }, }, setup(props) { - const { info } = inject(useApplicationKey); + const { info } = inject(InjectionKeys.useApplication); const appName = computed(() => info.name); diff --git a/src/presentation/components/Code/CodeButtons/TheCodeButtons.vue b/src/presentation/components/Code/CodeButtons/TheCodeButtons.vue index 422f62b3..1f80442d 100644 --- a/src/presentation/components/Code/CodeButtons/TheCodeButtons.vue +++ b/src/presentation/components/Code/CodeButtons/TheCodeButtons.vue @@ -29,7 +29,7 @@ import { defineComponent, ref, computed, inject, } from 'vue'; -import { useCollectionStateKey, useRuntimeEnvironmentKey } from '@/presentation/injectionSymbols'; +import { InjectionKeys } from '@/presentation/injectionSymbols'; import { SaveFileDialog, FileType } from '@/infrastructure/SaveFileDialog'; import { Clipboard } from '@/infrastructure/Clipboard'; import ModalDialog from '@/presentation/components/Shared/Modal/ModalDialog.vue'; @@ -53,9 +53,10 @@ export default defineComponent({ }, setup() { const { - currentState, currentContext, onStateChange, events, - } = inject(useCollectionStateKey)(); - const { os, isDesktop } = inject(useRuntimeEnvironmentKey); + currentState, currentContext, onStateChange, + } = inject(InjectionKeys.useCollectionState)(); + const { os, isDesktop } = inject(InjectionKeys.useRuntimeEnvironment); + const { events } = inject(InjectionKeys.useAutoUnsubscribedEvents)(); const areInstructionsVisible = ref(false); const canRun = computed(() => getCanRunState(currentState.value.os, isDesktop, os)); @@ -81,15 +82,18 @@ export default defineComponent({ } onStateChange((newState) => { + updateCurrentCode(newState.code.current); subscribeToCodeChanges(newState.code); }, { immediate: true }); function subscribeToCodeChanges(code: IApplicationCode) { - hasCode.value = code.current && code.current.length > 0; - events.unsubscribeAll(); - events.register(code.changed.on((newCode) => { - hasCode.value = newCode && newCode.code.length > 0; - })); + events.unsubscribeAllAndRegister([ + code.changed.on((newCode) => updateCurrentCode(newCode.code)), + ]); + } + + function updateCurrentCode(code: string) { + hasCode.value = code && code.length > 0; } async function getCurrentCode(): Promise { diff --git a/src/presentation/components/Code/TheCodeArea.vue b/src/presentation/components/Code/TheCodeArea.vue index 007c8cab..f50dbb18 100644 --- a/src/presentation/components/Code/TheCodeArea.vue +++ b/src/presentation/components/Code/TheCodeArea.vue @@ -13,7 +13,7 @@ import { defineComponent, onUnmounted, onMounted, inject, } from 'vue'; -import { useCollectionStateKey } from '@/presentation/injectionSymbols'; +import { InjectionKeys } from '@/presentation/injectionSymbols'; import { ICodeChangedEvent } from '@/application/Context/State/Code/Event/ICodeChangedEvent'; import { IScript } from '@/domain/IScript'; import { ScriptingLanguage } from '@/domain/ScriptingLanguage'; @@ -37,7 +37,8 @@ export default defineComponent({ NonCollapsing, }, setup(props) { - const { onStateChange, currentState, events } = inject(useCollectionStateKey)(); + const { onStateChange, currentState } = inject(InjectionKeys.useCollectionState)(); + const { events } = inject(InjectionKeys.useAutoUnsubscribedEvents)(); const editorId = 'codeEditor'; let editor: ace.Ace.Editor | undefined; @@ -61,19 +62,20 @@ export default defineComponent({ newState.collection.scripting.language, ); const appCode = newState.code; - const innerCode = appCode.current || getDefaultCode(newState.collection.scripting.language); - editor.setValue(innerCode, 1); - events.unsubscribeAll(); - events.register(appCode.changed.on((code) => updateCode(code))); + updateCode(appCode.current, newState.collection.scripting.language); + events.unsubscribeAllAndRegister([ + appCode.changed.on((code) => handleCodeChange(code)), + ]); } - function updateCode(event: ICodeChangedEvent) { + function updateCode(code: string, language: ScriptingLanguage) { + const innerCode = code || getDefaultCode(language); + editor.setValue(innerCode, 1); + } + + function handleCodeChange(event: ICodeChangedEvent) { removeCurrentHighlighting(); - if (event.isEmpty()) { - const defaultCode = getDefaultCode(currentState.value.collection.scripting.language); - editor.setValue(defaultCode, 1); - return; - } + updateCode(event.code, currentState.value.collection.scripting.language); editor.setValue(event.code, 1); if (event.addedScripts?.length > 0) { reactToChanges(event, event.addedScripts); diff --git a/src/presentation/components/Scripts/Menu/Selector/TheSelector.vue b/src/presentation/components/Scripts/Menu/Selector/TheSelector.vue index f1cd6331..368bd7fd 100644 --- a/src/presentation/components/Scripts/Menu/Selector/TheSelector.vue +++ b/src/presentation/components/Scripts/Menu/Selector/TheSelector.vue @@ -66,9 +66,10 @@