Skip to content

Conversation

@medbenmakhlouf
Copy link
Contributor

Summary
This Pull Request introduces significant improvements to the Highcharts wrapper API, focusing on simplifying the chart update logic and giving developers granular control over the rendering lifecycle.

By moving away from "black-box" input bindings, we are streamlining API usage and exposing the native Highcharts instance directly. This allows for more performant and predictable chart updates

Key Changes

  • Deprecation: Marked [(update)] and [(oneToOne)] inputs as deprecated.
  • Simplified Logic: Shifted the responsibility of triggering updates from the wrapper's internal state checking to the developer via manual .update() calls.

⚠️ Deprecation Notice
The following inputs are now deprecated and will be removed in a future release. Please migrate to the new chartInstance approach described below. [(update)] and [(oneToOne)]

💻 Usage Example
Instead of relying on two-way binding to trigger updates, developers should now capture the chart instance and call the .update() method manually (e.g., on a button click or signal effect).

HTML Template:

<highcharts-chart
    constructorType="chart"
    [options]="options()"
    (chartInstance)="chartInstance = $event">
</highcharts-chart>

<button (click)="redraw()">Redraw Chart</button>
export class GraphComponent {
  // Store the reference to the chart
  chartInstance: Highcharts.Chart | undefined = undefined;

  redraw() {
    if (this.chartInstance) {
      // Manual control: update(options, redraw, oneToOne)
      this.chartInstance.update(this.options(), true, true);
    }
  }
}

@medbenmakhlouf
Copy link
Contributor Author

@karolkolodziej Thanks in advance for the review! 😉

@medbenmakhlouf
Copy link
Contributor Author

fix : #428

@karolkolodziej
Copy link
Member

We always assumed the wrapper would handle updates based on the input, and now that responsibility shifts to the implementer.
At first glance, it looks fine, but I need a few days to test it more thoroughly.

@karolkolodziej
Copy link
Member

I really appreciate the work here — this PR is top-notch and everything functions as expected.

That said, this wrapper has a broad user base that relies heavily on the existing input bindings. Deprecating them entirely feels like a big breaking change with a large impact.
While I understand and agree with the motivation behind this approach, I think the current use case is already partially covered by exposing chartInstance, which allows users to manually trigger updates when they need more control.

Because of that, I’m concerned this change may be too aggressive at this stage.
A more gradual transition or dual support might be safer for existing consumers.

@medbenmakhlouf
Copy link
Contributor Author

@karolkolodziej I completely agree with your assessment. Pushing this change as v5.3.0 is too aggressive given the user base reliance on the existing bindings.

Proposed Action Plan:

  • Revert the previous problematic change: I've noticed significant user frustration regarding the changes from the previous PR Prevent chart updates before creation in highcharts-chart.directive.ts #425. I propose we revert that immediately to restore stability and trust for current users.

  • Modify this PR for Dual Support: For this current PR, I will rework it to support both the legacy inputs (marked as @deprecated) and the new architecture. This allows users to opt-in gradually without breaking their apps.

  • Target v6.0.0 for Cleanup: We can maintain this backward compatibility until May (aligned with Angular v19 EOL/v20 release). At that point, we release v6.0.0, where we can safely remove the deprecated inputs and the reverted logic in favor of the new, cleaner implementation.

Does this roadmap sound safer to you? If you agree, I can handle the revert and update this PR accordingly.

@karolkolodziej
Copy link
Member

All sounds excellent. I agree 100% with the approach.
Thanks a lot for taking this on — much appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants