From 14cbbb1d747bcb0d85886a945bdac0f20acd20e1 Mon Sep 17 00:00:00 2001 From: Simon Mumenthaler Date: Mon, 30 Mar 2026 12:48:01 +0200 Subject: [PATCH 1/4] feat(svg): expose `SvgBaseDirective` to allow custom icon component implementations --- .../src/lib/svg/svg-base.directive.ts | 61 +++++++++++++++++++ libs/components/src/lib/svg/svg.component.ts | 60 +++--------------- libs/components/src/public-api.ts | 1 + 3 files changed, 72 insertions(+), 50 deletions(-) create mode 100644 libs/components/src/lib/svg/svg-base.directive.ts diff --git a/libs/components/src/lib/svg/svg-base.directive.ts b/libs/components/src/lib/svg/svg-base.directive.ts new file mode 100644 index 00000000..5a097949 --- /dev/null +++ b/libs/components/src/lib/svg/svg-base.directive.ts @@ -0,0 +1,61 @@ +import { HttpErrorResponse } from '@angular/common/http' +import { Directive, effect, ElementRef, inject, Renderer2, Signal } from '@angular/core' +import { Logger } from '@shiftcode/logger' + +import { SvgRegistry } from './svg-registry.service' + +/** + * Base class for components that display an SVG element inline. + * The SVG content is directly inlined as a child of the component, so that CSS styles can easily be applied to it. + */ +@Directive() +export abstract class SvgBaseDirective { + protected readonly elRef = inject>(ElementRef) + protected readonly renderer = inject(Renderer2) + protected readonly svgRegistry = inject(SvgRegistry) + + protected abstract readonly logger: Logger + protected abstract data: Signal<{ url: string; attrs?: Record }> + + constructor() { + effect(() => { + const { url, attrs } = this.data() + this.getAndSet(url, attrs) + }) + } + + private getAndSet(url: string, attrs?: Record) { + this.svgRegistry + .getFromUrl(url) + .then(this.getSvgModifyFn(attrs)) + .then(this.setSvgElement) + .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) + } else { + this.logger.error(`Error retrieving icon for path ${url}`, err) + } + }) + } + + private readonly getSvgModifyFn = (attrs?: Record) => { + const attrsEntries = attrs ? Object.entries(attrs) : [] + if (attrsEntries.length === 0) { + return (svg: SVGElement): SVGElement => svg + } + + return (svg: SVGElement): SVGElement => { + for (const [key, val] of attrsEntries) { + svg.setAttribute(key, val) + } + return svg + } + } + + 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) + } +} diff --git a/libs/components/src/lib/svg/svg.component.ts b/libs/components/src/lib/svg/svg.component.ts index d985e191..d5a30bc7 100644 --- a/libs/components/src/lib/svg/svg.component.ts +++ b/libs/components/src/lib/svg/svg.component.ts @@ -1,21 +1,19 @@ -import { HttpErrorResponse } from '@angular/common/http' -import { ChangeDetectionStrategy, Component, effect, ElementRef, inject, input, Renderer2 } from '@angular/core' +import { ChangeDetectionStrategy, Component, computed, inject, input } from '@angular/core' import { Logger } from '@shiftcode/logger' import { LoggerService } from '@shiftcode/ngx-core' -import { SvgRegistry } from './svg-registry.service' +import { SvgBaseDirective } from './svg-base.directive' /** * Standalone SvgComponent to display svg inline. - * (Initially copied from material MdIcon Directive but got rid of unused functionality and refactored to Promises) * * - Specify the url input to load an SVG icon from a URL. * The SVG content is directly inlined as a child of the component, * so that CSS styles can easily be applied to it. - * The URL is loaded via an XMLHttpRequest, so it must be on the same domain as the page or its + * The URL is loaded via Angular's {@link HttpClient}, it must be on the same domain as the page or its * server must be configured to allow cross-domain requests. * @example - * + * */ @Component({ selector: 'sc-svg', @@ -24,52 +22,14 @@ import { SvgRegistry } from './svg-registry.service' styleUrls: ['./svg.component.scss'], changeDetection: ChangeDetectionStrategy.OnPush, }) -export class SvgComponent { +export class SvgComponent extends SvgBaseDirective { readonly url = input.required() readonly attrs = input>({}) - protected readonly elRef: ElementRef = inject(ElementRef) - protected readonly renderer = inject(Renderer2) - protected readonly svgRegistry = inject(SvgRegistry) - - private readonly logger: Logger = inject(LoggerService).getInstance('SvgComponent') - - constructor() { - effect(() => { - this.loadAndSetSvg(this.url(), this.attrs()) - }) - } - - private loadAndSetSvg(url: string, attrs: Record) { - if (!url.endsWith('.svg')) { - this.logger.warn('svg url does not end with *.svg') - } - this.svgRegistry - .getFromUrl(url) - .then(this.modifySvgElement(attrs)) - .then(this.setSvgElement) - .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 ${this.url()}, due to no network`, err) - } else { - this.logger.error(`Error retrieving icon for path ${this.url()}`, err) - } - }) - } - - private modifySvgElement(attrs: Record) { - return (svg: SVGElement): SVGElement => { - Object.keys(attrs).forEach((key) => svg.setAttribute(key, attrs[key])) - return svg - } - } - - private setSvgElement = (svg: SVGElement | null) => { - const layoutElement = this.elRef.nativeElement - // Remove existing child nodes and add the new SVG element. - layoutElement.innerHTML = '' - this.renderer.appendChild(layoutElement, svg) - } + protected readonly logger: Logger = inject(LoggerService).getInstance('SvgComponent') + protected readonly data = computed(() => ({ + url: this.url(), + attrs: this.attrs(), + })) } diff --git a/libs/components/src/public-api.ts b/libs/components/src/public-api.ts index 7a221513..1dab23e5 100644 --- a/libs/components/src/public-api.ts +++ b/libs/components/src/public-api.ts @@ -1,5 +1,6 @@ // svg export * from './lib/svg/svg.component' +export * from './lib/svg/svg-base.directive' export * from './lib/svg/svg-registry.service' // svg-animate From 4c985eba7cbb656ad2bf9436a0ca2424a6aed84b Mon Sep 17 00:00:00 2001 From: Github Actions Date: Mon, 30 Mar 2026 10:50:07 +0000 Subject: [PATCH 2/4] build(release): next version [skip_build] --- apps/styleguide/package.json | 2 +- lerna.json | 2 +- libs/components/package.json | 2 +- libs/core/package.json | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/apps/styleguide/package.json b/apps/styleguide/package.json index 2ff98cc5..5738e388 100644 --- a/apps/styleguide/package.json +++ b/apps/styleguide/package.json @@ -1,6 +1,6 @@ { "name": "@shiftcode/styleguide", - "version": "15.0.0", + "version": "15.1.0-pr80.0", "private": true, "type": "module", "scripts": { diff --git a/lerna.json b/lerna.json index b1180b0c..91f51b7b 100644 --- a/lerna.json +++ b/lerna.json @@ -2,7 +2,7 @@ "$schema": "node_modules/lerna/schemas/lerna-schema.json", "useNx": false, "packages": ["libs/*", "apps/*"], - "version": "15.0.0", + "version": "15.1.0-pr80.0", "command": { "version": { "allowBranch": "*", diff --git a/libs/components/package.json b/libs/components/package.json index e42fae9d..7844b061 100644 --- a/libs/components/package.json +++ b/libs/components/package.json @@ -1,6 +1,6 @@ { "name": "@shiftcode/ngx-components", - "version": "15.0.0", + "version": "15.1.0-pr80.0", "repository": "https://github.com/shiftcode/sc-ng-commons-public", "license": "MIT", "author": "shiftcode GmbH ", diff --git a/libs/core/package.json b/libs/core/package.json index 3b98db70..a75658c7 100644 --- a/libs/core/package.json +++ b/libs/core/package.json @@ -1,6 +1,6 @@ { "name": "@shiftcode/ngx-core", - "version": "15.0.0", + "version": "15.1.0-pr80.0", "repository": "https://github.com/shiftcode/sc-ng-commons-public", "license": "MIT", "author": "shiftcode GmbH ", From aa0032bca7c13d7e1fc9f95c35b0af97ffae1570 Mon Sep 17 00:00:00 2001 From: Simon Mumenthaler Date: Mon, 30 Mar 2026 13:45:25 +0200 Subject: [PATCH 3/4] feat(svg): enhance `SvgBaseDirective` to prevent race conditions --- .../src/lib/svg/svg-base.directive.spec.ts | 202 ++++++++++++++++++ .../src/lib/svg/svg-base.directive.ts | 34 +-- 2 files changed, 224 insertions(+), 12 deletions(-) create mode 100644 libs/components/src/lib/svg/svg-base.directive.spec.ts diff --git a/libs/components/src/lib/svg/svg-base.directive.spec.ts b/libs/components/src/lib/svg/svg-base.directive.spec.ts new file mode 100644 index 00000000..303d6cd7 --- /dev/null +++ b/libs/components/src/lib/svg/svg-base.directive.spec.ts @@ -0,0 +1,202 @@ +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 }>({ url: MOCK_SVG_URL }) + protected readonly logger = inject(Logger) +} + +function createSvgElement(): SVGElement { + const div = document.createElement('div') + div.innerHTML = `` + 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>() } + + TestBed.configureTestingModule({ + providers: [ + { provide: Logger, useValue: mockLogger }, + { provide: SvgRegistry, useValue: mockRegistry }, + ], + }) + + return { mockLogger, mockRegistry } +} + +async function flushMicrotasks(count = 2): Promise { + for (let i = 0; i < count; i++) { + await Promise.resolve() + } +} + +async function renderComponent(url: string = MOCK_SVG_URL, attrs?: Record) { + const fixture = TestBed.createComponent(TestSvgComponent) + + fixture.componentInstance.data.set({ url, attrs }) + TestBed.tick() + await flushMicrotasks() + + return { + fixture, + hostElement: fixture.nativeElement as HTMLElement, + } +} + +describe('SvgBaseDirective', () => { + test('fetches SVG from the registry and inserts it into the DOM', async () => { + const { mockRegistry } = setup() + const svg = createSvgElement() + mockRegistry.getFromUrl.mockResolvedValue(svg) + + const { hostElement } = await renderComponent() + + expect(mockRegistry.getFromUrl).toHaveBeenCalledWith(MOCK_SVG_URL) + expect(hostElement.contains(svg)).toBe(true) + }) + + test('applies provided attrs to the SVG element', async () => { + const { mockRegistry } = setup() + const svg = createSvgElement() + mockRegistry.getFromUrl.mockResolvedValue(svg) + + const { hostElement } = await 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', async () => { + 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 } = await 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', async () => { + const { mockRegistry } = setup() + const svgFirst = createSvgElement() + const svgSecond = createSvgElement() + + mockRegistry.getFromUrl.mockResolvedValueOnce(svgFirst).mockResolvedValueOnce(svgSecond) + + const { fixture, hostElement } = await renderComponent() + + expect(hostElement.contains(svgFirst)).toBe(true) + + fixture.componentInstance.data.set({ url: '/assets/other.svg' }) + TestBed.tick() + await flushMicrotasks() + + expect(hostElement.contains(svgSecond)).toBe(true) + }) + + test('clears existing DOM content before inserting the new SVG on signal change', async () => { + const { mockRegistry } = setup() + const svgFirst = createSvgElement() + const svgSecond = createSvgElement() + mockRegistry.getFromUrl.mockResolvedValueOnce(svgFirst).mockResolvedValueOnce(svgSecond) + + const { fixture, hostElement } = await renderComponent() + + expect(hostElement.contains(svgFirst)).toBe(true) + + fixture.componentInstance.data.set({ url: '/assets/other.svg' }) + TestBed.tick() + await flushMicrotasks() + + 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', async () => { + const { mockRegistry } = setup() + + let resolveFirst!: (svg: SVGElement) => void + const firstPromise = new Promise((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 } = await renderComponent() + + expect(mockRegistry.getFromUrl).toHaveBeenCalledTimes(1) + + // Change signal before firstPromise resolves + fixture.componentInstance.data.set({ url: '/assets/other.svg' }) + TestBed.tick() + await flushMicrotasks() + + // 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() + await flushMicrotasks() + expect(hostElement.contains(secondSvg)).toBe(true) + }) + + test('logs a debug info for network errors (HttpErrorResponse with status 0)', async () => { + const { mockLogger, mockRegistry } = setup() + + const networkError = new HttpErrorResponse({ status: 0, url: MOCK_SVG_URL }) + mockRegistry.getFromUrl.mockRejectedValue(networkError) + + await 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', async () => { + const { mockLogger, mockRegistry } = setup() + + const notFoundErrorResponse = new HttpErrorResponse({ status: 404, url: MOCK_SVG_URL }) + mockRegistry.getFromUrl.mockRejectedValue(notFoundErrorResponse) + + await renderComponent() + + expect(mockLogger.error).toHaveBeenCalledWith(expect.stringContaining(MOCK_SVG_URL), notFoundErrorResponse) + }) + + test('logs an error for non-HTTP errors', async () => { + const { mockLogger, mockRegistry } = setup() + + const genericError = new Error('Something went wrong') + mockRegistry.getFromUrl.mockRejectedValue(genericError) + + await renderComponent() + + expect(mockLogger.error).toHaveBeenCalledWith(expect.stringContaining(MOCK_SVG_URL), genericError) + }) +}) diff --git a/libs/components/src/lib/svg/svg-base.directive.ts b/libs/components/src/lib/svg/svg-base.directive.ts index 5a097949..931dd4c9 100644 --- a/libs/components/src/lib/svg/svg-base.directive.ts +++ b/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 }> + protected abstract readonly data: Signal<{ url: string; attrs?: Record }> 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) { + private getAndSet(url: string, attrs: Record | 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) => { + private getSvgModifierFn(attrs?: Record) { 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) + } } } From db4a30dfa6c75b2a4aa6786b48c4164b0e5399e2 Mon Sep 17 00:00:00 2001 From: Github Actions Date: Mon, 30 Mar 2026 12:28:16 +0000 Subject: [PATCH 4/4] build(release): next version [skip_build] --- apps/styleguide/package.json | 2 +- lerna.json | 2 +- libs/components/package.json | 2 +- libs/core/package.json | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/apps/styleguide/package.json b/apps/styleguide/package.json index 5738e388..ebd8ad5a 100644 --- a/apps/styleguide/package.json +++ b/apps/styleguide/package.json @@ -1,6 +1,6 @@ { "name": "@shiftcode/styleguide", - "version": "15.1.0-pr80.0", + "version": "15.1.0-pr80.1", "private": true, "type": "module", "scripts": { diff --git a/lerna.json b/lerna.json index 91f51b7b..6ffe6aff 100644 --- a/lerna.json +++ b/lerna.json @@ -2,7 +2,7 @@ "$schema": "node_modules/lerna/schemas/lerna-schema.json", "useNx": false, "packages": ["libs/*", "apps/*"], - "version": "15.1.0-pr80.0", + "version": "15.1.0-pr80.1", "command": { "version": { "allowBranch": "*", diff --git a/libs/components/package.json b/libs/components/package.json index 7844b061..44f60578 100644 --- a/libs/components/package.json +++ b/libs/components/package.json @@ -1,6 +1,6 @@ { "name": "@shiftcode/ngx-components", - "version": "15.1.0-pr80.0", + "version": "15.1.0-pr80.1", "repository": "https://github.com/shiftcode/sc-ng-commons-public", "license": "MIT", "author": "shiftcode GmbH ", diff --git a/libs/core/package.json b/libs/core/package.json index a75658c7..0f25ec78 100644 --- a/libs/core/package.json +++ b/libs/core/package.json @@ -1,6 +1,6 @@ { "name": "@shiftcode/ngx-core", - "version": "15.1.0-pr80.0", + "version": "15.1.0-pr80.1", "repository": "https://github.com/shiftcode/sc-ng-commons-public", "license": "MIT", "author": "shiftcode GmbH ",