feat(svg): enhance SvgBaseDirective to prevent race conditions

Author: mumenthalers

libs/components/src/lib/svg/svg-base.directive.spec.ts

@@ -0,0 +1,191 @@
+import { HttpErrorResponse } from '@angular/common/http'
+import { Component, inject, signal } from '@angular/core'
+import { TestBed } from '@angular/core/testing'
+import { Logger } from '@shiftcode/logger'
+import { describe, expect, test, vi } from 'vitest'
+
+import { SvgBaseDirective } from './svg-base.directive'
+import { SvgRegistry } from './svg-registry.service'
+
+const MOCK_SVG_URL = '/assets/test.svg'
+
+@Component({ selector: 'sc-test-svg-base', template: '', standalone: true })
+class TestSvgComponent extends SvgBaseDirective {
+  readonly data = signal<{ url: string; attrs?: Record<string, string> }>({ url: MOCK_SVG_URL })
+  protected readonly logger = inject(Logger)
+}
+
+function createSvgElement(): SVGElement {
+  const div = document.createElement('div')
+  div.innerHTML = `<svg xmlns="http://www.w3.org/2000/svg" data-ts="${Date.now()}"><path d="M0 0"/></svg>`
+  return div.querySelector('svg') as SVGElement
+}
+
+function setup() {
+  const mockLogger = {
+    debug: vi.fn(),
+    info: vi.fn(),
+    warn: vi.fn(),
+    error: vi.fn(),
+  } as unknown as Logger
+
+  const mockRegistry = { getFromUrl: vi.fn<() => Promise<SVGElement>>() }
+
+  TestBed.configureTestingModule({
+    providers: [
+      { provide: Logger, useValue: mockLogger },
+      { provide: SvgRegistry, useValue: mockRegistry },
+    ],
+  })
+
+  return { mockLogger, mockRegistry }
+}
+
+function renderComponent(url: string = MOCK_SVG_URL, attrs?: Record<string, string>) {
+  const fixture = TestBed.createComponent(TestSvgComponent)
+
+  fixture.componentInstance.data.set({ url, attrs })
+  TestBed.tick()
+
+  return {
+    fixture,
+    hostElement: fixture.nativeElement as HTMLElement,
+  }
+}
+
+describe('SvgBaseDirective', () => {
+  test('fetches SVG from the registry and inserts it into the DOM', () => {
+    const { mockRegistry } = setup()
+    const svg = createSvgElement()
+    mockRegistry.getFromUrl.mockResolvedValue(svg)
+
+    const { hostElement } = renderComponent()
+
+    expect(mockRegistry.getFromUrl).toHaveBeenCalledWith(MOCK_SVG_URL)
+    expect(hostElement.contains(svg)).toBe(true)
+  })
+
+  test('applies provided attrs to the SVG element', () => {
+    const { mockRegistry } = setup()
+    const svg = createSvgElement()
+    mockRegistry.getFromUrl.mockResolvedValue(svg)
+
+    const { hostElement } = renderComponent(MOCK_SVG_URL, { 'data-test': 'true', class: 'my-icon' })
+
+    expect(svg.getAttribute('data-test')).toBe('true')
+    expect(svg.getAttribute('class')).toBe('my-icon')
+    expect(hostElement.contains(svg)).toBe(true)
+  })
+
+  test('does not modify SVG attributes when no attrs are provided', () => {
+    const { mockRegistry } = setup()
+    const svg = createSvgElement()
+    const originalAttributes = Array.from(svg.attributes).map((a) => ({ name: a.name, value: a.value }))
+    mockRegistry.getFromUrl.mockResolvedValue(svg)
+
+    const { hostElement } = renderComponent()
+
+    expect(Array.from(svg.attributes).map((a) => ({ name: a.name, value: a.value }))).toEqual(originalAttributes)
+    expect(hostElement.contains(svg)).toBe(true)
+  })
+
+  test('allows to update the SVG when the url signal changes', () => {
+    const { mockRegistry } = setup()
+    const svgFirst = createSvgElement()
+    const svgSecond = createSvgElement()
+
+    mockRegistry.getFromUrl.mockResolvedValueOnce(svgFirst).mockResolvedValueOnce(svgSecond)
+
+    const { fixture, hostElement } = renderComponent()
+
+    expect(hostElement.contains(svgFirst)).toBe(true)
+
+    fixture.componentInstance.data.set({ url: '/assets/other.svg' })
+    TestBed.tick()
+
+    expect(hostElement.contains(svgSecond)).toBe(true)
+  })
+
+  test('clears existing DOM content before inserting the new SVG on signal change', () => {
+    const { mockRegistry } = setup()
+    const svgFirst = createSvgElement()
+    const svgSecond = createSvgElement()
+    mockRegistry.getFromUrl.mockResolvedValueOnce(svgFirst).mockResolvedValueOnce(svgSecond)
+
+    const { fixture, hostElement } = renderComponent()
+
+    expect(hostElement.contains(svgFirst)).toBe(true)
+
+    fixture.componentInstance.data.set({ url: '/assets/other.svg' })
+    TestBed.tick()
+
+    expect(hostElement.contains(svgFirst)).toBe(false)
+    expect(hostElement.contains(svgSecond)).toBe(true)
+    expect(hostElement.querySelectorAll('svg').length).toBe(1)
+  })
+
+  test('does not insert SVG when the signal changes before the first request resolved', () => {
+    const { mockRegistry } = setup()
+
+    let resolveFirst!: (svg: SVGElement) => void
+    const firstPromise = new Promise<SVGElement>((resolve) => (resolveFirst = resolve))
+
+    const firstSvg = createSvgElement()
+    const secondSvg = createSvgElement()
+
+    mockRegistry.getFromUrl
+      .mockReturnValueOnce(firstPromise) // first call returns a promise that we can control
+      .mockResolvedValueOnce(secondSvg) // second call resolves immediately
+
+    const { fixture, hostElement } = renderComponent()
+
+    expect(mockRegistry.getFromUrl).toHaveBeenCalledTimes(1)
+
+    // Change signal before firstPromise resolves
+    fixture.componentInstance.data.set({ url: '/assets/other.svg' })
+    TestBed.tick()
+
+    // At this point, the second svg should be rendered.
+    expect(mockRegistry.getFromUrl).toHaveBeenCalledTimes(2)
+    expect(hostElement.contains(secondSvg)).toBe(true)
+
+    // Now resolve the first promise; it should have no effect in the DOM anymore.
+    resolveFirst(firstSvg)
+    TestBed.tick()
+    expect(hostElement.contains(secondSvg)).toBe(true)
+  })
+
+  test('logs a debug info for network errors (HttpErrorResponse with status 0)', () => {
+    const { mockLogger, mockRegistry } = setup()
+
+    const networkError = new HttpErrorResponse({ status: 0, url: MOCK_SVG_URL })
+    mockRegistry.getFromUrl.mockRejectedValue(networkError)
+
+    renderComponent()
+
+    expect(mockLogger.debug).toHaveBeenCalledWith(expect.stringContaining(MOCK_SVG_URL), networkError)
+    expect(mockLogger.error).not.toHaveBeenCalled()
+  })
+
+  test('logs an error for HttpErrorResponse with a non-zero status', () => {
+    const { mockLogger, mockRegistry } = setup()
+
+    const notFoundErrorResponse = new HttpErrorResponse({ status: 404, url: MOCK_SVG_URL })
+    mockRegistry.getFromUrl.mockRejectedValue(notFoundErrorResponse)
+
+    renderComponent()
+
+    expect(mockLogger.error).toHaveBeenCalledWith(expect.stringContaining(MOCK_SVG_URL), notFoundErrorResponse)
+  })
+
+  test('logs an error for non-HTTP errors', () => {
+    const { mockLogger, mockRegistry } = setup()
+
+    const genericError = new Error('Something went wrong')
+    mockRegistry.getFromUrl.mockRejectedValue(genericError)
+
+    renderComponent()
+
+    expect(mockLogger.error).toHaveBeenCalledWith(expect.stringContaining(MOCK_SVG_URL), genericError)
+  })
+})

libs/components/src/lib/svg/svg-base.directive.ts

@@ -15,31 +15,35 @@ export abstract class SvgBaseDirective {
   protected readonly svgRegistry = inject(SvgRegistry)
 
   protected abstract readonly logger: Logger
-  protected abstract data: Signal<{ url: string; attrs?: Record<string, string> }>
+  protected abstract readonly data: Signal<{ url: string; attrs?: Record<string, string> }>
 
   constructor() {
-    effect(() => {
+    effect((onCleanup) => {
       const { url, attrs } = this.data()
-      this.getAndSet(url, attrs)
+      const abortController = new AbortController()
+      this.getAndSet(url, attrs, abortController.signal)
+      onCleanup(() => abortController.abort())
     })
   }
 
-  private getAndSet(url: string, attrs?: Record<string, string>) {
+  private getAndSet(url: string, attrs: Record<string, string> | undefined, abortSignal: AbortSignal) {
+    // due to the caching in SvgRegistry we cannot simply abort the fetching of the svg.
+    // but we ensure that we do not set the svg element if the abort signal has been triggered in the meantime.
     this.svgRegistry
       .getFromUrl(url)
-      .then(this.getSvgModifyFn(attrs))
-      .then(this.setSvgElement)
+      .then(this.getSvgModifierFn(attrs))
+      .then(this.getSvgSetterFn(abortSignal))
       .catch((err: any) => {
         if (err instanceof HttpErrorResponse && err.status === 0) {
           // in case of no internet or a timeout log a warning, we can not do anything about that
-          this.logger.warn(`Error retrieving icon for path ${url}, due to no network`, err)
+          this.logger.debug(`Error retrieving icon for path ${url}, due to no network`, err)
         } else {
           this.logger.error(`Error retrieving icon for path ${url}`, err)
         }
       })
   }
 
-  private readonly getSvgModifyFn = (attrs?: Record<string, string>) => {
+  private getSvgModifierFn(attrs?: Record<string, string>) {
     const attrsEntries = attrs ? Object.entries(attrs) : []
     if (attrsEntries.length === 0) {
       return (svg: SVGElement): SVGElement => svg
@@ -53,9 +57,15 @@ export abstract class SvgBaseDirective {
     }
   }
 
-  private readonly setSvgElement = (svg: SVGElement | null) => {
-    // Remove existing child nodes and add the new SVG element.
-    this.elRef.nativeElement.innerHTML = ''
-    this.renderer.appendChild(this.elRef.nativeElement, svg)
+  private getSvgSetterFn = (abortSignal: AbortSignal) => {
+    return (svg: SVGElement) => {
+      if (abortSignal.aborted) {
+        this.logger.debug('Aborting setSvgElement due to abort signal')
+        return
+      }
+      // Remove existing child nodes and add the new SVG element.
+      this.elRef.nativeElement.innerHTML = ''
+      this.renderer.appendChild(this.elRef.nativeElement, svg)
+    }
   }
 }