Fix: makeDraggable memory leak (#3624)

Co-authored-by: wangfukang <[email protected]>
katspaugh · Apr 3, 2024 · f023a72 · f023a72
1 parent ab4abee
commit f023a72
Show file tree

Hide file tree

Showing 3 changed files with 77 additions and 58 deletions.
diff --git a/src/plugins/envelope.ts b/src/plugins/envelope.ts
@@ -55,10 +55,12 @@ class Polyline extends EventEmitter<{
       circle: SVGEllipseElement
     }
   >
+  private subscriptions: (() => void)[] = []
 
   constructor(options: Options, wrapper: HTMLElement) {
     super()
 
+    this.subscriptions = []
     this.options = options
     this.polyPoints = new Map()
 
@@ -109,21 +111,23 @@ class Polyline extends EventEmitter<{
 
     // Make the polyline draggable along the Y axis
     if (options.dragLine) {
-      makeDraggable(polyline as unknown as HTMLElement, (_, dy) => {
-        const { height } = svg.viewBox.baseVal
-        const { points } = polyline
-        for (let i = 1; i < points.numberOfItems - 1; i++) {
-          const point = points.getItem(i)
-          point.y = Math.min(height, Math.max(0, point.y + dy))
-        }
-        const circles = svg.querySelectorAll('ellipse')
-        Array.from(circles).forEach((circle) => {
-          const newY = Math.min(height, Math.max(0, Number(circle.getAttribute('cy')) + dy))
-          circle.setAttribute('cy', newY.toString())
-        })
-
-        this.emit('line-move', dy / height)
-      })
+      this.subscriptions.push(
+        makeDraggable(polyline as unknown as HTMLElement, (_, dy) => {
+          const { height } = svg.viewBox.baseVal
+          const { points } = polyline
+          for (let i = 1; i < points.numberOfItems - 1; i++) {
+            const point = points.getItem(i)
+            point.y = Math.min(height, Math.max(0, point.y + dy))
+          }
+          const circles = svg.querySelectorAll('ellipse')
+          Array.from(circles).forEach((circle) => {
+            const newY = Math.min(height, Math.max(0, Number(circle.getAttribute('cy')) + dy))
+            circle.setAttribute('cy', newY.toString())
+          })
+
+          this.emit('line-move', dy / height)
+        }),
+      )
     }
 
     // Listen to double click to add a new point
@@ -161,12 +165,14 @@ class Polyline extends EventEmitter<{
   }
 
   private makeDraggable(draggable: SVGElement, onDrag: (x: number, y: number) => void) {
-    makeDraggable(
-      draggable as unknown as HTMLElement,
-      onDrag,
-      () => (draggable.style.cursor = 'grabbing'),
-      () => (draggable.style.cursor = 'grab'),
-      1,
+    this.subscriptions.push(
+      makeDraggable(
+        draggable as unknown as HTMLElement,
+        onDrag,
+        () => (draggable.style.cursor = 'grabbing'),
+        () => (draggable.style.cursor = 'grab'),
+        1,
+      ),
     )
   }
 
@@ -267,6 +273,7 @@ class Polyline extends EventEmitter<{
   }
 
   destroy() {
+    this.subscriptions.forEach((unsubscribe) => unsubscribe())
     this.polyPoints.clear()
     this.svg.remove()
   }

diff --git a/src/plugins/regions.ts b/src/plugins/regions.ts
@@ -78,10 +78,12 @@ class SingleRegion extends EventEmitter<RegionEvents> {
   public maxLength = Infinity
   public channelIdx: number
   public contentEditable = false
+  public subscriptions: (() => void)[] = []
 
   constructor(params: RegionParams, private totalDuration: number, private numberOfChannels = 0) {
     super()
 
+    this.subscriptions = []
     this.id = params.id || `region-${Math.random().toString(32).slice(2)}`
     this.start = this.clampPosition(params.start)
     this.end = this.clampPosition(params.end ?? params.start)
@@ -150,19 +152,21 @@ class SingleRegion extends EventEmitter<RegionEvents> {
 
     // Resize
     const resizeThreshold = 1
-    makeDraggable(
-      leftHandle,
-      (dx) => this.onResize(dx, 'start'),
-      () => null,
-      () => this.onEndResizing(),
-      resizeThreshold,
-    )
-    makeDraggable(
-      rightHandle,
-      (dx) => this.onResize(dx, 'end'),
-      () => null,
-      () => this.onEndResizing(),
-      resizeThreshold,
+    this.subscriptions.push(
+      makeDraggable(
+        leftHandle,
+        (dx) => this.onResize(dx, 'start'),
+        () => null,
+        () => this.onEndResizing(),
+        resizeThreshold,
+      ),
+      makeDraggable(
+        rightHandle,
+        (dx) => this.onResize(dx, 'end'),
+        () => null,
+        () => this.onEndResizing(),
+        resizeThreshold,
+      ),
     )
   }
 
@@ -235,14 +239,16 @@ class SingleRegion extends EventEmitter<RegionEvents> {
     element.addEventListener('pointerup', () => this.toggleCursor(false))
 
     // Drag
-    makeDraggable(
-      element,
-      (dx) => this.onMove(dx),
-      () => this.toggleCursor(true),
-      () => {
-        this.toggleCursor(false)
-        this.drag && this.emit('update-end')
-      },
+    this.subscriptions.push(
+      makeDraggable(
+        element,
+        (dx) => this.onMove(dx),
+        () => this.toggleCursor(true),
+        () => {
+          this.toggleCursor(false)
+          this.drag && this.emit('update-end')
+        },
+      ),
     )
 
     if (this.contentEditable && this.content) {
@@ -379,6 +385,7 @@ class SingleRegion extends EventEmitter<RegionEvents> {
   /** Remove the region */
   public remove() {
     this.emit('remove')
+    this.subscriptions.forEach((unsubscribe) => unsubscribe())
     this.element.remove()
     // This violates the type but we want to clean up the DOM reference
     // w/o having to have a nullable type of the element

diff --git a/src/renderer.ts b/src/renderer.ts
@@ -29,10 +29,12 @@ class Renderer extends EventEmitter<RendererEvents> {
   private resizeObserver: ResizeObserver | null = null
   private lastContainerWidth = 0
   private isDragging = false
+  private subscriptions: (() => void)[] = []
 
   constructor(options: WaveSurferOptions, audioElement?: HTMLElement) {
     super()
 
+    this.subscriptions = []
     this.options = options
 
     const parent = this.parentFromOptionsContainer(options.container)
@@ -122,22 +124,24 @@ class Renderer extends EventEmitter<RendererEvents> {
   }
 
   private initDrag() {
-    makeDraggable(
-      this.wrapper,
-      // On drag
-      (_, __, x) => {
-        this.emit('drag', Math.max(0, Math.min(1, x / this.wrapper.getBoundingClientRect().width)))
-      },
-      // On start drag
-      (x) => {
-        this.isDragging = true
-        this.emit('dragstart', Math.max(0, Math.min(1, x / this.wrapper.getBoundingClientRect().width)))
-      },
-      // On end drag
-      (x) => {
-        this.isDragging = false
-        this.emit('dragend', Math.max(0, Math.min(1, x / this.wrapper.getBoundingClientRect().width)))
-      },
+    this.subscriptions.push(
+      makeDraggable(
+        this.wrapper,
+        // On drag
+        (_, __, x) => {
+          this.emit('drag', Math.max(0, Math.min(1, x / this.wrapper.getBoundingClientRect().width)))
+        },
+        // On start drag
+        (x) => {
+          this.isDragging = true
+          this.emit('dragstart', Math.max(0, Math.min(1, x / this.wrapper.getBoundingClientRect().width)))
+        },
+        // On end drag
+        (x) => {
+          this.isDragging = false
+          this.emit('dragend', Math.max(0, Math.min(1, x / this.wrapper.getBoundingClientRect().width)))
+        },
+      ),
     )
   }
 
@@ -268,6 +272,7 @@ class Renderer extends EventEmitter<RendererEvents> {
   }
 
   destroy() {
+    this.subscriptions.forEach((unsubscribe) => unsubscribe())
     this.container.remove()
     this.resizeObserver?.disconnect()
   }