bugfix/437-performance-regression

[AndrzejBuleczka]

Fixed performance regression on multiple charts render, closes #437.

[AndrzejBuleczka]

I reworked the v5 init path so the directive no longer relies on the old default 500ms timeout as a synchronization buffer. Instead of calling a fire-and-forget load() and then waiting before creating the chart, HighchartsChartService.load(partialConfig) now returns a real readiness promise for that directive’s exact config: it caches the shared Highcharts core/global setup once, loads partial modules for the current directive, and resolves only when that specific instance is ready to use. On the directive side, chart creation is now driven by that readiness signal rather than by an async computed() with a default wait, so we keep the async lifecycle boundary but remove the artificial startup penalty that was amplifying the regression on pages with many complex charts. In practice this restores behavior much closer to v4 because we are coordinating on actual module readiness instead of time-based guessing.

[wchmiel]

I’ve tested the changes locally and it looks like the solution works and the bug has been fixed – well done ;) However, I think @karolkolodziej or someone involved more in maintaining the wrapper should check this, as there are some core changes here. I just have a few minor questions/requests.

[KacperMadej]

Trashing time-based guessing for loading is good improvement.

I'm still a bit rusty with this repo and a lot has changed since I was actively involved in the development, so please take my review with a grain of salt.

Could you point me to the performance test that we could use in the future to make sure it's not dropping back down after, let's say HC v13 release?

With HC v13 being available currently as a beta release - have you tested this PR with it? It's not a must have but would be nice to check this in advance - there can be a follow up task to take care of this to avoid blocking this PR.

[AndrzejBuleczka]

Thanks for the review! 🙏

On the performance test: there isn't a timing benchmark in the repo today, and a wall-clock one would likely be flaky in CI. Instead I leaned on structural guards that lock in the fix: should reuse the shared Highcharts load across multiple load calls (asserts the loader, global modules, and setOptions each run exactly once regardless of chart count) and should create the chart as soon as Highcharts is loaded (guards against reintroducing a fixed delay). If those stay green we won't silently regress back to per-chart loading or a time-based delay. Happy to add a dedicated render-N-charts benchmark as a follow-up if you'd like a harder guard.

On v13, I tested against that directly. Both the lib and app test suites pass unchanged against v13, including the real dynamic-import module tests (map, tilemap, gantt, dumbbell, pattern-fill), so the ESM import paths and module constructor names still resolve. No code changes were needed. I'd suggest bumping the dependency range to allow v13 as a small follow-up.

[medbenmakhlouf]

@AndrzejBuleczka, I think this.destroyRef.destroyed can be used here instead of maintaining a separate isDestroyed flag.

[AndrzejBuleczka]

Good idea in principle, but DestroyRef only exposes onDestroy() — there's no public destroyed property in Angular 19, which is the minimum version this library supports (@angular/core: >=19.0.0). So I've kept the isDestroyed flag for now. Happy to switch to a built-in once we raise the minimum Angular version.

[KacperMadej]

On the performance test: there isn't a timing benchmark in the repo today, and a wall-clock one would likely be flaky in CI. Instead I leaned on structural guards that lock in the fix: should reuse the shared Highcharts load across multiple load calls (asserts the loader, global modules, and setOptions each run exactly once regardless of chart count) and should create the chart as soon as Highcharts is loaded (guards against reintroducing a fixed delay). If those stay green we won't silently regress back to per-chart loading or a time-based delay. Happy to add a dedicated render-N-charts benchmark as a follow-up if you'd like a harder guard.

✅ The issue is about performance so it would be nice to have anything to back up the claim it is better now and also something to help with the maintenance. I'm ok with what you described and something more could be added if needed in the future.

On v13, I tested against that directly. Both the lib and app test suites pass unchanged against v13, including the real dynamic-import module tests (map, tilemap, gantt, dumbbell, pattern-fill), so the ESM import paths and module constructor names still resolve. No code changes were needed. I'd suggest bumping the dependency range to allow v13 as a small follow-up.

👍 Distributed package.json has "peerDependencies": { ... "highcharts": ">=12.2.0" so this is only about this repo - please bump the package and no release would be required for this part.

👀 Other that that, please take a look at the comments above from Mohamed.

The merge-base changed after approval.

The merge-base changed after approval.

[KacperMadej]

👍

The merge-base changed after approval.

[KacperMadej]

This PR is bugged and full of small commits. Please create a fresh branch from the master one with only the needed commits and we could merge that PR and close this one.

[KacperMadej]

Hi @hakimio , GH is doing it due to some settings and branch-magic, details linked in my previous message (and also here). We'll get this sorted out this week and we can plan a new release next week to get this change published.

[AndrzejBuleczka]

ok, closing this one, new PR with these changes ready