From 8c0aaef65685b6730e08375261d27540ecfd9361 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andrzej=20Bu=C5=82eczka?= Date: Wed, 1 Jul 2026 09:57:26 +0200 Subject: [PATCH 1/2] Fixed performance regression on multiple charts render, closes #437. --- README.md | 7 +- .../lib/highcharts-chart.component.spec.ts | 1 - .../lib/highcharts-chart.directive.spec.ts | 31 +++- .../src/lib/highcharts-chart.directive.ts | 168 +++++++++++------- .../lib/highcharts-chart.performance.spec.ts | 115 ++++++++++++ .../src/lib/highcharts-chart.service.spec.ts | 84 +++++---- .../src/lib/highcharts-chart.service.ts | 77 +++++--- highcharts-angular/src/lib/types.ts | 6 +- package.json | 2 +- 9 files changed, 364 insertions(+), 127 deletions(-) create mode 100644 highcharts-angular/src/lib/highcharts-chart.performance.spec.ts diff --git a/README.md b/README.md index 300a865..1f0eb32 100644 --- a/README.md +++ b/README.md @@ -104,7 +104,7 @@ export const appConfig: ApplicationConfig = { ]; }, - // Optional: override the default 500ms delay before chart creation + // Optional: defer chart creation even after Highcharts is ready timeout: 0, }), // Other providers here ... @@ -358,7 +358,7 @@ import { HighchartsChartDirective } from 'highcharts-angular'; import('highcharts/esm/modules/exporting'), ]; }, - timeout: 900, // Optional: increase timeout for loading modules + timeout: 900, // Optional: defer chart creation after module loading }), ], }) @@ -443,8 +443,7 @@ export class StockComponent { **Note:** - Some Highcharts modules have dependencies and must be loaded in a specific order. -- In such cases, use a promise chain (e.g., `import('highcharts/esm/highcharts-more').then(() => import('highcharts/esm/modules/dumbbell'))`) -- instead of just listing them as array items. This ensures the dependent module loads only after its dependency. +- Use a promise chain instead of listing dependent modules as separate array items, for example `import('highcharts/esm/highcharts-more').then(() => import('highcharts/esm/modules/dumbbell'))`. For `tilemap`, load `map` first: `import('highcharts/esm/modules/map').then(() => import('highcharts/esm/modules/tilemap'))`. ### To load a wrapper diff --git a/highcharts-angular/src/lib/highcharts-chart.component.spec.ts b/highcharts-angular/src/lib/highcharts-chart.component.spec.ts index a2ed374..4661b5e 100644 --- a/highcharts-angular/src/lib/highcharts-chart.component.spec.ts +++ b/highcharts-angular/src/lib/highcharts-chart.component.spec.ts @@ -134,7 +134,6 @@ describe('TestComponent / HighchartsChartService (load module)', () => { providers: [ providePartialHighcharts({ modules: () => [ - import('highcharts/esm/modules/map'), import('highcharts/esm/modules/map').then(() => import('highcharts/esm/modules/tilemap')), import('highcharts/esm/modules/gantt'), import('highcharts/esm/highcharts-more').then(() => import('highcharts/esm/modules/dumbbell')), diff --git a/highcharts-angular/src/lib/highcharts-chart.directive.spec.ts b/highcharts-angular/src/lib/highcharts-chart.directive.spec.ts index 2c8a8d9..915e1b1 100644 --- a/highcharts-angular/src/lib/highcharts-chart.directive.spec.ts +++ b/highcharts-angular/src/lib/highcharts-chart.directive.spec.ts @@ -1,4 +1,4 @@ -import { TestBed } from '@angular/core/testing'; +import { ComponentFixture, TestBed } from '@angular/core/testing'; import { ChangeDetectionStrategy, Component, DebugElement } from '@angular/core'; import { By } from '@angular/platform-browser'; import { HighchartsChartDirective } from './highcharts-chart.directive'; @@ -18,12 +18,27 @@ class TestHostComponent { } describe('HighchartsChartDirective', () => { + let fixture: ComponentFixture; let debugElement: DebugElement; let directive: HighchartsChartDirective; let loadSpy: Spy; + let chartFactorySpy: Spy; beforeEach(() => { - loadSpy = jasmine.createSpy('load'); + chartFactorySpy = jasmine.createSpy('chart').and.returnValue({ + destroy: jasmine.createSpy('destroy'), + renderer: { + forExport: false, + }, + update: jasmine.createSpy('update'), + } as unknown as Partial); + + const mockHighcharts = { + chart: chartFactorySpy, + } as unknown as typeof Highcharts; + + loadSpy = jasmine.createSpy('load').and.resolveTo(mockHighcharts); + TestBed.configureTestingModule({ imports: [TestHostComponent, HighchartsChartDirective], providers: [ @@ -35,13 +50,12 @@ describe('HighchartsChartDirective', () => { provide: HighchartsChartService, useValue: { load: loadSpy, - highcharts: () => null, }, }, ], }); - const fixture = TestBed.createComponent(TestHostComponent); + fixture = TestBed.createComponent(TestHostComponent); fixture.detectChanges(); debugElement = fixture.debugElement.query(By.directive(HighchartsChartDirective)); directive = debugElement.injector.get(HighchartsChartDirective); @@ -58,4 +72,13 @@ describe('HighchartsChartDirective', () => { it('should load global config on initialization', () => { expect(loadSpy).toHaveBeenCalledWith({}); }); + + it('should create the chart as soon as Highcharts is loaded', async () => { + expect(chartFactorySpy).not.toHaveBeenCalled(); + + await fixture.whenStable(); + fixture.detectChanges(); + + expect(chartFactorySpy).toHaveBeenCalled(); + }); }); diff --git a/highcharts-angular/src/lib/highcharts-chart.directive.ts b/highcharts-angular/src/lib/highcharts-chart.directive.ts index adc7f4a..1547d5d 100644 --- a/highcharts-angular/src/lib/highcharts-chart.directive.ts +++ b/highcharts-angular/src/lib/highcharts-chart.directive.ts @@ -1,5 +1,4 @@ import { - computed, DestroyRef, Directive, effect, @@ -8,13 +7,15 @@ import { input, model, output, - untracked, + PendingTasks, PLATFORM_ID, + signal, + untracked, } from '@angular/core'; import { isPlatformServer } from '@angular/common'; import { HighchartsChartService } from './highcharts-chart.service'; import { HIGHCHARTS_CONFIG, HIGHCHARTS_TIMEOUT } from './highcharts-chart.token'; -import { ChartConstructorType, ConstructorChart } from './types'; +import { ChartConstructorType, ConstructorChart, HighchartsWithModuleConstructors } from './types'; import type Highcharts from 'highcharts/esm/highcharts'; @Directive({ @@ -23,6 +24,7 @@ import type Highcharts from 'highcharts/esm/highcharts'; export class HighchartsChartDirective { /** * Type of the chart constructor. + * Changing it recreates the chart because Highcharts constructors are not update-compatible. */ public readonly constructorType = input('chart'); @@ -47,94 +49,136 @@ export class HighchartsChartDirective { */ public readonly update = model(true); - public readonly chartInstance = output(); // #26 + public readonly chartInstance = output(); private readonly destroyRef = inject(DestroyRef); - private readonly el = inject>(ElementRef); - private readonly platformId = inject(PLATFORM_ID); - - private readonly relativeConfig = inject(HIGHCHARTS_CONFIG, { - optional: true, - }); - - private readonly timeout = inject(HIGHCHARTS_TIMEOUT, { - optional: true, - }); - + private readonly relativeConfig = inject(HIGHCHARTS_CONFIG, { optional: true }); + private readonly timeout = inject(HIGHCHARTS_TIMEOUT, { optional: true }); private readonly highchartsChartService = inject(HighchartsChartService); + private readonly pendingTasks = inject(PendingTasks); - private chartCreated = false; - - private _chartInstance: Highcharts.Chart | undefined; - + private readonly loadedHighcharts = signal(null); + private readonly chart = signal(null); private isDestroyed = false; private delay(ms: number): Promise { return new Promise(resolve => setTimeout(resolve, ms)); } - // Create the chart as soon as we can - private readonly chart = computed(async () => { - const highCharts = this.highchartsChartService.highcharts(); - const constructorType = this.constructorType(); - await this.delay(this.relativeConfig?.timeout ?? this.timeout ?? 500); - if (!highCharts) return; - const callback: Highcharts.ChartCallbackFunction = (chart: Highcharts.Chart) => { - if (chart.renderer.forExport || this.isDestroyed) return; - return this.chartInstance.emit(chart); - }; - const chartFactories: Record = { - chart: highCharts.chart, - ganttChart: (highCharts as any).ganttChart, - mapChart: (highCharts as any).mapChart, - stockChart: (highCharts as any).stockChart, - }; - return chartFactories[constructorType]( - this.el.nativeElement, - // Use untracked, so we don't re-create new chart everytime options change - untracked(() => this.options()), - // Use Highcharts callback to emit chart instance, so it is available as early - // as possible. So that Angular is already aware of the instance if Highcharts raise - // events during the initialization that happens before coming back to Angular - callback, - ); - }); + private getChartFactory(highcharts: typeof Highcharts, constructorType: ChartConstructorType): ConstructorChart { + if (constructorType === 'chart') { + return highcharts.chart; + } + + const highchartsWithModuleConstructors: HighchartsWithModuleConstructors = highcharts; + const chartFactory = highchartsWithModuleConstructors[constructorType]; + + if (!chartFactory) { + throw new Error( + `Highcharts constructor "${constructorType}" is not available. Did you load the required module?`, + ); + } + + return chartFactory; + } + + private createChart(): void { + effect(onCleanup => { + const highcharts = this.loadedHighcharts(); + const constructorType = this.constructorType(); + + if (!highcharts || this.isDestroyed) { + return; + } + + const callback: Highcharts.ChartCallbackFunction = (chart: Highcharts.Chart) => { + if (chart.renderer.forExport || this.isDestroyed) return; + return this.chartInstance.emit(chart); + }; + + const chart = this.getChartFactory(highcharts, constructorType)( + this.el.nativeElement, + // Read options without tracking them here: option changes should update + // the existing chart, not tear it down and create a new one. + untracked(() => this.options()), + callback, + ); + + this.chart.set(chart); + + onCleanup(() => { + if (this.chart() === chart) { + this.chart.set(null); + } + + chart.destroy(); + }); + }); + } private keepChartUpToDate(): void { - effect(async () => { + let lastChart: Highcharts.Chart | null = null; + + effect(() => { + const chart = this.chart(); + // Deprecated inputs remain supported internally until they are removed. + // eslint-disable-next-line @typescript-eslint/no-deprecated const update = this.update(); + // eslint-disable-next-line @typescript-eslint/no-deprecated const oneToOne = this.oneToOne(); const options = this.options(); - this._chartInstance = await this.chart(); - if (!this.chartCreated) { - if (this._chartInstance) { - this.chartCreated = true; - } - } else { - if (update) { - this._chartInstance?.update(options, true, oneToOne); + + if (!chart) { + return; + } + + // Skip the first pass after creation. The constructor already consumed + // the initial options, so calling `update` immediately would duplicate work. + if (chart !== lastChart) { + lastChart = chart; + return; + } + + if (update) { + chart.update(options, true, oneToOne); + } + }); + } + + private async initializeHighcharts(): Promise { + await this.pendingTasks.run(async () => { + try { + const highcharts = await this.highchartsChartService.load(this.relativeConfig); + const delayMs = this.relativeConfig?.timeout ?? this.timeout ?? 0; + + // Keep a timer boundary even at 0ms so Angular test stability and + // component timing stay aligned with the directive's async setup. + await this.delay(delayMs); + + if (!this.isDestroyed) { + this.loadedHighcharts.set(highcharts); } + } catch (error) { + // Core/module loading failed: leave the chart uncreated and surface the + // reason instead of letting it become an unhandled promise rejection. + console.error('Highcharts failed to load; chart was not created.', error); } }); } public constructor() { - // should stop loading on the server side for SSR if (this.platformId && isPlatformServer(this.platformId)) { return; } - // make sure to load global config + modules on demand - this.highchartsChartService.load(this.relativeConfig); - // destroy the chart when the directive is destroyed + this.destroyRef.onDestroy(() => { - this._chartInstance?.destroy(); - this._chartInstance = undefined; this.isDestroyed = true; - }); // #44 + }); - // Keep the chart up to date whenever options change or the update special input is set to true + this.createChart(); this.keepChartUpToDate(); + void this.initializeHighcharts(); } } diff --git a/highcharts-angular/src/lib/highcharts-chart.performance.spec.ts b/highcharts-angular/src/lib/highcharts-chart.performance.spec.ts new file mode 100644 index 0000000..1d01e40 --- /dev/null +++ b/highcharts-angular/src/lib/highcharts-chart.performance.spec.ts @@ -0,0 +1,115 @@ +import { ComponentFixture, TestBed } from '@angular/core/testing'; +import { ChangeDetectionStrategy, Component, input } from '@angular/core'; +import { HighchartsChartDirective } from './highcharts-chart.directive'; +import { HighchartsChartService } from './highcharts-chart.service'; +import { + HIGHCHARTS_CONFIG, + HIGHCHARTS_LOADER, + HIGHCHARTS_OPTIONS, + HIGHCHARTS_ROOT_MODULES, +} from './highcharts-chart.token'; +import { InstanceFactoryFunction, ModuleFactoryFunction } from './types'; +import Spy = jasmine.Spy; +import type Highcharts from 'highcharts/esm/highcharts'; + +/** + * Scaling guard for the #437 performance fix. + * + * The regression was structural — the library was loaded and initialized per + * chart instead of once — so this test renders N charts against the real + * {@link HighchartsChartService} and asserts that shared initialization stays + * O(1) (loader, global modules and setOptions each run exactly once) while + * per-chart work stays O(N) (one factory call per chart, no redundant + * re-creations). It fails loudly if per-chart loading or a fixed delay is + * reintroduced. + */ +@Component({ + selector: 'highcharts-perf-host', + template: ` + @for (i of charts(); track i) { +
+ } + `, + imports: [HighchartsChartDirective], + changeDetection: ChangeDetectionStrategy.OnPush, +}) +class PerfHostComponent { + public readonly count = input(0); + protected readonly charts = (): number[] => Array.from({ length: this.count() }, (_, i) => i); +} + +describe('HighchartsChartDirective performance scaling', () => { + let fixture: ComponentFixture; + let loaderSpy: Spy; + let globalModulesSpy: Spy; + let setOptionsSpy: Spy; + let chartFactorySpy: Spy; + + async function renderCharts(count: number): Promise { + fixture.componentRef.setInput('count', count); + fixture.detectChanges(); + await fixture.whenStable(); + fixture.detectChanges(); + } + + beforeEach(() => { + setOptionsSpy = jasmine.createSpy('setOptions'); + chartFactorySpy = jasmine.createSpy('chart').and.callFake( + () => + ({ + destroy: jasmine.createSpy('destroy'), + renderer: { forExport: false }, + update: jasmine.createSpy('update'), + }) as unknown as Highcharts.Chart, + ); + + const mockHighcharts = { + chart: chartFactorySpy, + setOptions: setOptionsSpy, + } as unknown as typeof Highcharts; + + loaderSpy = jasmine.createSpy('loader').and.resolveTo(mockHighcharts); + globalModulesSpy = jasmine.createSpy('globalModules').and.returnValue([]); + + TestBed.configureTestingModule({ + imports: [PerfHostComponent, HighchartsChartDirective], + providers: [ + HighchartsChartService, + { provide: HIGHCHARTS_LOADER, useValue: loaderSpy }, + { provide: HIGHCHARTS_ROOT_MODULES, useValue: globalModulesSpy }, + { provide: HIGHCHARTS_OPTIONS, useValue: { lang: { thousandsSep: ',' } } }, + { provide: HIGHCHARTS_CONFIG, useValue: {} }, + ], + }); + + fixture = TestBed.createComponent(PerfHostComponent); + }); + + it('shares Highcharts initialization across many charts (init once, one factory call per chart)', async () => { + const chartCount = 25; + + await renderCharts(chartCount); + + // Shared initialization must not scale with the number of charts. + expect(loaderSpy).toHaveBeenCalledTimes(1); + expect(globalModulesSpy).toHaveBeenCalledTimes(1); + expect(setOptionsSpy).toHaveBeenCalledTimes(1); + + // Per-chart work scales linearly: exactly one factory call per chart. + expect(chartFactorySpy).toHaveBeenCalledTimes(chartCount); + }); + + it('keeps shared initialization constant as the chart count grows', async () => { + await renderCharts(5); + await renderCharts(40); + + // Adding more charts does not re-run the one-time shared setup... + expect(loaderSpy).toHaveBeenCalledTimes(1); + expect(globalModulesSpy).toHaveBeenCalledTimes(1); + expect(setOptionsSpy).toHaveBeenCalledTimes(1); + + // ...and the original 5 charts are reused rather than recreated, so the + // factory runs once per distinct chart (40), not once per render pass (45). + expect(chartFactorySpy).toHaveBeenCalledTimes(40); + }); +}); diff --git a/highcharts-angular/src/lib/highcharts-chart.service.spec.ts b/highcharts-angular/src/lib/highcharts-chart.service.spec.ts index 67e692e..5087ca3 100644 --- a/highcharts-angular/src/lib/highcharts-chart.service.spec.ts +++ b/highcharts-angular/src/lib/highcharts-chart.service.spec.ts @@ -1,4 +1,4 @@ -import { TestBed, tick, fakeAsync } from '@angular/core/testing'; +import { TestBed } from '@angular/core/testing'; import { HighchartsChartService } from './highcharts-chart.service'; import { HIGHCHARTS_LOADER, HIGHCHARTS_OPTIONS, HIGHCHARTS_ROOT_MODULES } from './highcharts-chart.token'; import { ModuleFactoryFunction, InstanceFactoryFunction } from './types'; @@ -8,21 +8,21 @@ describe('HighchartsChartService', () => { let service: HighchartsChartService; let mockLoader: typeof Highcharts; let mockGlobalOptions: Highcharts.Options; - let mockGlobalModules: ModuleFactoryFunction; + let mockGlobalModules: jasmine.Spy; + let mockLoaderInstance: jasmine.Spy; beforeEach(() => { - // Mock Highcharts instance with the setOptions method mockLoader = { setOptions: jasmine.createSpy('setOptions'), } as unknown as typeof Highcharts; mockGlobalOptions = { lang: { thousandsSep: ',' } }; - mockGlobalModules = jasmine.createSpy('mockGlobalModules').and.returnValue([]); - const instance = (): Promise => Promise.resolve(mockLoader); + mockGlobalModules = jasmine.createSpy('mockGlobalModules').and.returnValue([]); + mockLoaderInstance = jasmine.createSpy('mockLoaderInstance').and.resolveTo(mockLoader); TestBed.configureTestingModule({ providers: [ HighchartsChartService, - { provide: HIGHCHARTS_LOADER, useValue: instance }, + { provide: HIGHCHARTS_LOADER, useValue: mockLoaderInstance }, { provide: HIGHCHARTS_OPTIONS, useValue: mockGlobalOptions }, { provide: HIGHCHARTS_ROOT_MODULES, useValue: mockGlobalModules }, ], @@ -35,41 +35,64 @@ describe('HighchartsChartService', () => { expect(service).toBeTruthy(); }); - it('should emit the loaded Highcharts instance', fakeAsync(() => { + it('should emit the loaded Highcharts instance', async () => { expect(service.highcharts()).toBeNull(); - service.load(null); - tick(100); // Simulate the passage of time for the timeout in the load method + await service.load(null); - expect(service.highcharts()).toBe(mockLoader); // The Highcharts object should be emitted - })); + expect(service.highcharts()).toBe(mockLoader); + }); - it('should call setOptions with global options if provided', fakeAsync(() => { - service.load(null); - tick(100); // Simulate the passage of time for the timeout in the load method + it('should call setOptions with global options if provided', async () => { + await service.load(null); - // Check if setOptions was called with the global options expect(mockLoader.setOptions).toHaveBeenCalledWith(mockGlobalOptions); - })); + }); - it('should load global modules if provided', fakeAsync(() => { - service.load(null); - tick(100); // Simulate the passage of time for the timeout in the load method + it('should load global modules if provided', async () => { + await service.load(null); - // Wait for each module to resolve, then check if its `default` method was called with highcharts expect(mockGlobalModules).toHaveBeenCalled(); - })); + }); - it('should load partialConfig modules if provided', fakeAsync(() => { - const mockPartialModules = jasmine.createSpy('mockPartialModules').and.returnValue([]); + it('should load partialConfig modules if provided', async () => { + const mockPartialModules = jasmine.createSpy('mockPartialModules').and.returnValue([]); - // Call the load method with partial modules - service.load({ modules: mockPartialModules }); - tick(100); // Simulate the passage of time for the timeout in the load method + await service.load({ modules: mockPartialModules }); - // Check if partial modules were loaded expect(mockPartialModules).toHaveBeenCalled(); - })); + }); + + it('should reuse the shared Highcharts load across multiple load calls', async () => { + const firstLoad = service.load(null); + const secondLoad = service.load(null); + + const [firstInstance, secondInstance] = await Promise.all([firstLoad, secondLoad]); + + expect(firstInstance).toBe(mockLoader); + expect(secondInstance).toBe(mockLoader); + expect(mockLoaderInstance).toHaveBeenCalledTimes(1); + expect(mockGlobalModules).toHaveBeenCalledTimes(1); + expect(mockLoader.setOptions).toHaveBeenCalledTimes(1); + }); + + it('should reuse identical partial module factories across load calls', async () => { + const mockPartialModules = jasmine.createSpy('mockPartialModules').and.returnValue([]); + + await Promise.all([service.load({ modules: mockPartialModules }), service.load({ modules: mockPartialModules })]); + + expect(mockPartialModules).toHaveBeenCalledTimes(1); + }); + + it('should report module load failures', async () => { + const mockPartialModules = jasmine + .createSpy('mockPartialModules') + .and.returnValue([Promise.reject(new Error('broken module'))]); + + await expectAsync(service.load({ modules: mockPartialModules })).toBeRejectedWithError( + 'Failed to load Highcharts modules: broken module', + ); + }); }); describe('With not provided Value', () => { @@ -97,9 +120,8 @@ describe('With not provided Value', () => { }); it('should not call setOptions if global options are not provided', async () => { - service.load(null); - const highcharts = await mockLoaderInstance(); + await service.load(null); - expect(highcharts.setOptions).not.toHaveBeenCalled(); + expect(mockLoader.setOptions).not.toHaveBeenCalled(); }); }); diff --git a/highcharts-angular/src/lib/highcharts-chart.service.ts b/highcharts-angular/src/lib/highcharts-chart.service.ts index 36227af..f94752c 100644 --- a/highcharts-angular/src/lib/highcharts-chart.service.ts +++ b/highcharts-angular/src/lib/highcharts-chart.service.ts @@ -1,6 +1,6 @@ import { inject, Injectable, signal } from '@angular/core'; import { HIGHCHARTS_ROOT_MODULES, HIGHCHARTS_LOADER, HIGHCHARTS_OPTIONS } from './highcharts-chart.token'; -import { PartialHighchartsConfig } from './types'; +import { ModuleFactoryFunction, PartialHighchartsConfig } from './types'; import type Highcharts from 'highcharts/esm/highcharts'; @Injectable({ providedIn: 'root' }) @@ -15,33 +15,66 @@ export class HighchartsChartService { optional: true, }); - private async loadHighchartsWithModules(partialConfig: PartialHighchartsConfig | null): Promise { - const highcharts = await this.loader(); // Ensure Highcharts core is loaded + private sharedHighchartsPromise: Promise | null = null; + private readonly moduleLoadCache = new WeakMap>(); - const moduleResults = await Promise.allSettled([ - ...(this.globalModules?.() ?? []), - ...(partialConfig?.modules?.() ?? []), - ]); - const rejectedModules = moduleResults.filter( - (result): result is PromiseRejectedResult => result.status === 'rejected', - ); - - if (rejectedModules.length) { - const reasons = rejectedModules.map(({ reason }) => (reason instanceof Error ? reason.message : String(reason))); + private async loadModules(modulesFactory?: ModuleFactoryFunction | null): Promise { + if (!modulesFactory) { + return; + } - throw new Error(`Failed to load Highcharts modules: ${reasons.join('; ')}`); + const cachedLoad = this.moduleLoadCache.get(modulesFactory); + if (cachedLoad) { + return cachedLoad; } - // Return the Highcharts instance - return highcharts; - } + const moduleLoad = Promise.allSettled(modulesFactory()).then(moduleResults => { + const rejectedModules = moduleResults.filter( + (result): result is PromiseRejectedResult => result.status === 'rejected', + ); + + if (rejectedModules.length) { + const reasons = rejectedModules.map(({ reason }) => + reason instanceof Error ? reason.message : String(reason), + ); - public load(partialConfig: PartialHighchartsConfig | null): void { - this.loadHighchartsWithModules(partialConfig).then(highcharts => { - if (this.globalOptions) { - highcharts.setOptions(this.globalOptions); + throw new Error(`Failed to load Highcharts modules: ${reasons.join('; ')}`); } - this.highcharts.set(highcharts); }); + + this.moduleLoadCache.set(modulesFactory, moduleLoad); + + return moduleLoad; + } + + private async ensureSharedHighcharts(): Promise { + if (!this.sharedHighchartsPromise) { + this.sharedHighchartsPromise = (async () => { + const highcharts = await this.loader(); + + // Root-level modules and options mutate a shared Highcharts singleton, + // so initialize them once and reuse the same ready instance afterwards. + await this.loadModules(this.globalModules); + + if (this.globalOptions) { + highcharts.setOptions(this.globalOptions); + } + + return highcharts; + })(); + } + + return this.sharedHighchartsPromise; + } + + public async load(partialConfig: PartialHighchartsConfig | null): Promise { + const highcharts = await this.ensureSharedHighcharts(); + + // Component-level modules are still loaded per config, but cached by the + // factory function so identical providers do not repeat the same work. + await this.loadModules(partialConfig?.modules); + + this.highcharts.set(highcharts); + return highcharts; } } diff --git a/highcharts-angular/src/lib/types.ts b/highcharts-angular/src/lib/types.ts index df60bb5..3f59642 100644 --- a/highcharts-angular/src/lib/types.ts +++ b/highcharts-angular/src/lib/types.ts @@ -2,6 +2,9 @@ import type Highcharts from 'highcharts/esm/highcharts'; export type ChartConstructorType = 'chart' | 'ganttChart' | 'stockChart' | 'mapChart'; +export type ModuleChartConstructorType = Exclude; +export type HighchartsWithModuleConstructors = typeof Highcharts & + Partial>; export type ModuleFactoryFunction = () => Promise[]; export type InstanceFactoryFunction = () => Promise; @@ -22,8 +25,7 @@ export type PartialHighchartsConfig = { */ modules?: ModuleFactoryFunction; /** - * Timeout in milliseconds to wait for the Highcharts library to load - * Default is 500ms + * Optional delay in milliseconds before chart creation after Highcharts and modules are ready */ timeout?: number; }; diff --git a/package.json b/package.json index a46e7a6..656592e 100644 --- a/package.json +++ b/package.json @@ -36,7 +36,7 @@ "@highcharts/map-collection": "^2.3.0", "core-js": "^3.19.2", "express": "^4.18.2", - "highcharts": "^12.2.0", + "highcharts": ">=12.2.0", "jquery": "^3.6.0", "prettier": "^3.5.3", "proj4": "^2.15.0", From 6498b242047b29e708edf81ddafd5d4dc8f2899e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andrzej=20Bu=C5=82eczka?= Date: Wed, 1 Jul 2026 10:34:07 +0200 Subject: [PATCH 2/2] Refactored, harden Highcharts load retries and clean up leftover timeout delay. --- README.md | 8 ++++---- .../lib/highcharts-chart.component.spec.ts | 4 +--- .../src/lib/highcharts-chart.directive.ts | 8 +++++--- .../src/lib/highcharts-chart.service.ts | 19 ++++++++++++++++++- 4 files changed, 28 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 1f0eb32..929e617 100644 --- a/README.md +++ b/README.md @@ -103,9 +103,6 @@ export const appConfig: ApplicationConfig = { import('highcharts/esm/themes/sunset'), ]; }, - - // Optional: defer chart creation even after Highcharts is ready - timeout: 0, }), // Other providers here ... ], @@ -358,7 +355,10 @@ import { HighchartsChartDirective } from 'highcharts-angular'; import('highcharts/esm/modules/exporting'), ]; }, - timeout: 900, // Optional: defer chart creation after module loading + // Optional escape hatch: extra delay (ms) before the chart is created, + // after Highcharts and modules are ready (e.g. to let the container settle). + // Rarely needed — module loading is awaited automatically. Defaults to 0. + timeout: 0, }), ], }) diff --git a/highcharts-angular/src/lib/highcharts-chart.component.spec.ts b/highcharts-angular/src/lib/highcharts-chart.component.spec.ts index 4661b5e..208f344 100644 --- a/highcharts-angular/src/lib/highcharts-chart.component.spec.ts +++ b/highcharts-angular/src/lib/highcharts-chart.component.spec.ts @@ -52,7 +52,6 @@ describe('TestComponent / HighchartsChartService (load module)', () => { title: string; modules: ModuleFactoryFunction; assert(hc: any): void; - timeout?: number; }; const CASES: Case[] = [ @@ -78,7 +77,6 @@ describe('TestComponent / HighchartsChartService (load module)', () => { expect(hc.seriesTypes?.arearange).toBeDefined(); expect(hc.seriesTypes?.dumbbell).toBeDefined(); }, - timeout: 2000, }, { title: 'pattern-fill → adds SVGRenderer.addPattern', @@ -104,7 +102,7 @@ describe('TestComponent / HighchartsChartService (load module)', () => { // 3) Provide the module(s) at the component level. This mirrors your real component’s // `providers: [providePartialHighcharts({ modules: () => [...] })]`. TestBed.overrideComponent(TestComponent, { - set: { providers: [providePartialHighcharts({ modules: c.modules, timeout: c.timeout })] }, + set: { providers: [providePartialHighcharts({ modules: c.modules })] }, }); const fixture = TestBed.createComponent(TestComponent); diff --git a/highcharts-angular/src/lib/highcharts-chart.directive.ts b/highcharts-angular/src/lib/highcharts-chart.directive.ts index 1547d5d..f095a0e 100644 --- a/highcharts-angular/src/lib/highcharts-chart.directive.ts +++ b/highcharts-angular/src/lib/highcharts-chart.directive.ts @@ -153,9 +153,11 @@ export class HighchartsChartDirective { const highcharts = await this.highchartsChartService.load(this.relativeConfig); const delayMs = this.relativeConfig?.timeout ?? this.timeout ?? 0; - // Keep a timer boundary even at 0ms so Angular test stability and - // component timing stay aligned with the directive's async setup. - await this.delay(delayMs); + // Optional escape hatch: defer chart creation after Highcharts is ready + // (e.g. to let the container settle). Skipped entirely by default. + if (delayMs > 0) { + await this.delay(delayMs); + } if (!this.isDestroyed) { this.loadedHighcharts.set(highcharts); diff --git a/highcharts-angular/src/lib/highcharts-chart.service.ts b/highcharts-angular/src/lib/highcharts-chart.service.ts index f94752c..88dd6b8 100644 --- a/highcharts-angular/src/lib/highcharts-chart.service.ts +++ b/highcharts-angular/src/lib/highcharts-chart.service.ts @@ -44,12 +44,20 @@ export class HighchartsChartService { this.moduleLoadCache.set(modulesFactory, moduleLoad); + // Drop the memo if the load fails so an identical factory can be retried + // instead of reusing a permanently rejected promise. + moduleLoad.catch(() => { + if (this.moduleLoadCache.get(modulesFactory) === moduleLoad) { + this.moduleLoadCache.delete(modulesFactory); + } + }); + return moduleLoad; } private async ensureSharedHighcharts(): Promise { if (!this.sharedHighchartsPromise) { - this.sharedHighchartsPromise = (async () => { + const load = (async () => { const highcharts = await this.loader(); // Root-level modules and options mutate a shared Highcharts singleton, @@ -62,6 +70,15 @@ export class HighchartsChartService { return highcharts; })(); + + // Cache the in-flight load, but drop the memo if it fails so a later + // load() can retry instead of reusing a permanently rejected promise. + this.sharedHighchartsPromise = load; + load.catch(() => { + if (this.sharedHighchartsPromise === load) { + this.sharedHighchartsPromise = null; + } + }); } return this.sharedHighchartsPromise;