From 54b90b499b40528d7cf90b5ca6bd0e70489964ba Mon Sep 17 00:00:00 2001 From: Andrei Costescu Date: Thu, 16 Apr 2026 23:42:28 +0300 Subject: [PATCH] Fixed issue 6265 It was a memory leak - the instantiated Uppy was never destroyed, leaking Uppy instances via document listeners that were never removed. See issue on GitHub for more details. --- .../dashboard-modal.component.ts | 2 +- .../dashboard/dashboard.component.ts | 2 +- .../status-bar/status-bar.component.ts | 2 +- .../uppy/angular/src/lib/utils/wrapper.ts | 64 +++++++++++++++---- 4 files changed, 56 insertions(+), 14 deletions(-) diff --git a/packages/@uppy/angular/projects/uppy/angular/src/lib/components/dashboard-modal/dashboard-modal.component.ts b/packages/@uppy/angular/projects/uppy/angular/src/lib/components/dashboard-modal/dashboard-modal.component.ts index d2e51baf6..301b5dc71 100644 --- a/packages/@uppy/angular/projects/uppy/angular/src/lib/components/dashboard-modal/dashboard-modal.component.ts +++ b/packages/@uppy/angular/projects/uppy/angular/src/lib/components/dashboard-modal/dashboard-modal.component.ts @@ -26,7 +26,7 @@ export class DashboardModalComponent { el = inject(ElementRef); - @Input() uppy: Uppy = new Uppy(); + @Input() uppy: Uppy = this.createInitialUppy(); @Input() props: DashboardOptions = {}; @Input() open: boolean = false; diff --git a/packages/@uppy/angular/projects/uppy/angular/src/lib/components/dashboard/dashboard.component.ts b/packages/@uppy/angular/projects/uppy/angular/src/lib/components/dashboard/dashboard.component.ts index f26f56901..ba2243d21 100644 --- a/packages/@uppy/angular/projects/uppy/angular/src/lib/components/dashboard/dashboard.component.ts +++ b/packages/@uppy/angular/projects/uppy/angular/src/lib/components/dashboard/dashboard.component.ts @@ -26,7 +26,7 @@ export class DashboardComponent { el = inject(ElementRef); - @Input() uppy: Uppy = new Uppy(); + @Input() uppy: Uppy = this.createInitialUppy(); @Input() props: DashboardOptions = {}; /** Inserted by Angular inject() migration for backwards compatibility */ diff --git a/packages/@uppy/angular/projects/uppy/angular/src/lib/components/status-bar/status-bar.component.ts b/packages/@uppy/angular/projects/uppy/angular/src/lib/components/status-bar/status-bar.component.ts index 090fac758..b43a25ed7 100644 --- a/packages/@uppy/angular/projects/uppy/angular/src/lib/components/status-bar/status-bar.component.ts +++ b/packages/@uppy/angular/projects/uppy/angular/src/lib/components/status-bar/status-bar.component.ts @@ -26,7 +26,7 @@ export class StatusBarComponent { el = inject(ElementRef); - @Input() uppy: Uppy = new Uppy(); + @Input() uppy: Uppy = this.createInitialUppy(); @Input() props: StatusBarOptions = {}; /** Inserted by Angular inject() migration for backwards compatibility */ diff --git a/packages/@uppy/angular/projects/uppy/angular/src/lib/utils/wrapper.ts b/packages/@uppy/angular/projects/uppy/angular/src/lib/utils/wrapper.ts index ef2281e94..3753446d6 100644 --- a/packages/@uppy/angular/projects/uppy/angular/src/lib/utils/wrapper.ts +++ b/packages/@uppy/angular/projects/uppy/angular/src/lib/utils/wrapper.ts @@ -1,6 +1,7 @@ import type { ElementRef, SimpleChanges } from "@angular/core"; -import type { UIPlugin, UIPluginOptions, Uppy, Body, Meta } from "@uppy/core"; +import type { UIPlugin, UIPluginOptions, Body, Meta } from "@uppy/core"; import type { DashboardOptions } from "@uppy/dashboard"; +import { Uppy } from "@uppy/core"; export abstract class UppyAngularWrapper< M extends Meta, @@ -13,6 +14,19 @@ export abstract class UppyAngularWrapper< abstract uppy: Uppy; private options: any; plugin: PluginType | undefined; + + private uppyInstanceManagedByThisClass?: Uppy; + + /** + * If extending classes need an initial instance of Uppy - use this one; it will be destroyed when needed to avoid memory leaks. + */ + protected createInitialUppy(): Uppy { + if (!this.uppyInstanceManagedByThisClass) + // the instance of Uppy that is created here - we are in charge of destroying it when it's no longer used + this.uppyInstanceManagedByThisClass = new Uppy(); + + return this.uppyInstanceManagedByThisClass; + } onMount( defaultOptions: Partial, @@ -28,18 +42,33 @@ export abstract class UppyAngularWrapper< } handleChanges(changes: SimpleChanges, plugin: any): void { - // Without the last part of this conditional, it tries to uninstall before the plugin is mounted - if ( - changes["uppy"] && - this.uppy !== changes["uppy"].previousValue && - changes["uppy"].previousValue !== undefined - ) { - this.uninstall(changes["uppy"].previousValue); - // @ts-expect-error The options correspond to the plugin, I swear - this.uppy.use(plugin, this.options); + if (changes["uppy"]) + { + // "uppy" should never get set to null/undefined - that is not supported + + // Without the undefined check below, initially it would try to uninstall before the plugin is mounted (ngOnChanges + // that calls this gets called before ngOnInit that calls onMount if uppy is an @Input in derived classes; so the + // "uppyInstanceManagedByThisClass" didn't even get to be used; and changes["uppy"].previousValue doesn't give + // that anyway initially, but undefined, as that value was a 'default') + if (this.uppy !== changes["uppy"].previousValue && changes["uppy"].previousValue !== undefined) { + // we had an actively used Uppy that now changed + this.uninstall(changes["uppy"].previousValue); // this also destroys old Uppy if needed + // @ts-expect-error The options correspond to the plugin, I swear + this.uppy.use(plugin, this.options); + } else { + // can get here from derived classes on the initial ngOnChanges call of @Input uppy for example; previousValue + // is undefined then, even if we did have a this.uppy from constructor (now replaced) that was not yet making + // use of the plugin; so we don't uninstall (that also does destroy) here, just destroy if needed + let previousValueIncludingDefault = changes["uppy"].previousValue ? changes["uppy"].previousValue + : this.uppyInstanceManagedByThisClass; + if (this.uppy !== previousValueIncludingDefault) + this.destroyUppyIfNeeed(previousValueIncludingDefault); + } } + this.options = { ...this.options, ...this.props }; this.plugin = this.uppy.getPlugin(this.options.id) as PluginType; + if ( changes["props"] && this.props !== changes["props"].previousValue && @@ -49,7 +78,20 @@ export abstract class UppyAngularWrapper< } } - uninstall(uppy = this.uppy): void { + uninstall(uppy: Uppy = this.uppy): void { uppy.removePlugin(this.plugin!); + this.destroyUppyIfNeeed(uppy); } + + private destroyUppyIfNeeed(uppyThatWillNoLongerBeUsed: Uppy): void { + // "uppyThatWillNoLongerBeUsed" is either the "uppyInstanceManagedByThisClass" or it's received via derived classes + // from the outside (as an "@input" for example); if the Uppy that this instance created is no longer used, + // we need to destroy it in order to avoid memory leaks + + if (uppyThatWillNoLongerBeUsed === this.uppyInstanceManagedByThisClass) { + this.uppyInstanceManagedByThisClass.destroy(); + delete this.uppyInstanceManagedByThisClass; + } // else whoever provided the Uppy instance (as @Input maybe) is in charge of destroying it + } + }