(feat) propose new release v5.0.0 #392
Conversation
|
@karolkolodziej thank you for your support :) , here are my answers to your comments
I’ve added documentation to clarify the change from component to directive, and I made sure we cover all the important aspects. Let me know if there's anything more you’d like to see or if something is still unclear.
I’ve moved the old documentation to a separate file README_V4 and started updating the README. I’ve tried to cover everything, but please let me know if I missed something or if there’s anything else you’d like to add
I’ve already set up a demo app with SSR, and I’ve made sure all our local showcases work fine. You can try it out with the command:
I’ve changed the component to use the OnPush strategy and everything is now based on Signals, so it should be safe to remove the
Yes, I have added support for v12.x.x, and it was actually quite good for me to bring a DX that works with both version 11 and 12. Check it out! |
Thank you for taking the time to explain this. I agree with that now, as is demonstrated by your example
I cannot agree with this one though. Build brokenRunning I removed all reference to that package to be able to test the PR. Not lazy loaded
Instead, I suggest we mirror what is done for modules and declare the token as a factory, like below, and adapt the existing code: diff --git highcharts-angular/src/lib/types.ts highcharts-angular/src/lib/types.ts
index 2fc8597..2a06e39 100644
--- highcharts-angular/src/lib/types.ts
+++ highcharts-angular/src/lib/types.ts
@@ -3,6 +3,7 @@ import type HighchartsESM from 'highcharts/es-modules/masters/highcharts.src';
export type ChartConstructorType = 'chart' | 'ganttChart' | 'stockChart' | 'mapChart';
+export type InstanceFactoryFunction = () => Promise<Chart['highcharts']>;
export type ModuleFactoryFunction = () => Promise<ModuleFactory>[];
export interface ModuleFactory {
diff --git highcharts-angular/src/lib/highcharts-chart.token.ts highcharts-angular/src/lib/highcharts-chart.token.ts
index 0c69eb7..fb0d2d1 100644
--- highcharts-angular/src/lib/highcharts-chart.token.ts
+++ highcharts-angular/src/lib/highcharts-chart.token.ts
@@ -1,7 +1,7 @@
import { InjectionToken } from '@angular/core';
-import { Chart, ModuleFactoryFunction, PartialHighchartsConfig } from './types'
+import { Chart, InstanceFactoryFunction, ModuleFactoryFunction, PartialHighchartsConfig } from './types'
-export const HIGHCHARTS_LOADER = new InjectionToken<Promise<Chart['highcharts']>>('HIGHCHARTS_LOADER');
+export const HIGHCHARTS_LOADER = new InjectionToken<InstanceFactoryFunction>('HIGHCHARTS_LOADER');
export const HIGHCHARTS_ROOT_MODULES = new InjectionToken<ModuleFactoryFunction>('HIGHCHARTS_ROOT_MODULES');
export const HIGHCHARTS_OPTIONS = new InjectionToken<Chart['options']>('HIGHCHARTS_OPTIONS');
export const HIGHCHARTS_CONFIG = new InjectionToken<PartialHighchartsConfig>('HIGHCHARTS_CONFIG');Drop CommonJSESM is the only correct way of consuming JS library in Angular ecosystem, and thus support for CJS is not welcome anymore. I suggest the following: diff --git highcharts-angular/src/lib/types.ts highcharts-angular/src/lib/types.ts
index 2fc8597..b9c46fa 100644
--- highcharts-angular/src/lib/types.ts
+++ highcharts-angular/src/lib/types.ts
@@ -1,4 +1,3 @@
-import type Highcharts from 'highcharts';
import type HighchartsESM from 'highcharts/es-modules/masters/highcharts.src';
export type ChartConstructorType = 'chart' | 'ganttChart' | 'stockChart' | 'mapChart';
@@ -11,9 +10,9 @@ export interface ModuleFactory {
}
export interface Chart {
- options: Highcharts.Options | HighchartsESM.Options,
+ options: HighchartsESM.Options,
update?: boolean,
- highcharts?: typeof Highcharts | typeof HighchartsESM
+ highcharts?: typeof HighchartsESM
constructorChart?: Function;Drop HighchartsChartModuleI don't see a reason to keep HighchartsChartModule in the next major version. What we may do is deprecated it in a patch version before the major version. But the major version really should drop it entirely. Is has outlived its purpose. Minimal, explicit, public API surfaceThe public API should be kept at a minimum to reduce maintenance and cost and not confuse end-users with things they are not supposed to use directly. Specifically no tokens should be public and internal providers should stay internal. I suggest this: diff --git highcharts-angular/src/public_api.ts highcharts-angular/src/public_api.ts
index c4f9103..e344c84 100644
--- highcharts-angular/src/public_api.ts
+++ highcharts-angular/src/public_api.ts
@@ -2,10 +2,14 @@
* Public API Surface of highcharts-angular
*/
-export * from './lib/highcharts-chart.module';
-export * from './lib/highcharts-chart.component';
-export * from './lib/highcharts-chart.directive';
-export * from './lib/highcharts-chart.token';
-export * from './lib/highcharts-chart.provider';
-export * from './lib/highcharts-chart.service';
-export * from './lib/types';
+export {
+ HighchartsChartComponent
+} from './lib/highcharts-chart.component';
+export {
+ HighchartsChartDirective
+} from './lib/highcharts-chart.directive';
+export {provideHighCharts, providePartialHighChart} from './lib/highcharts-chart.provider';
+export {
+ HighchartsConfig,
+ PartialHighchartsConfig,
+} from './lib/types'; |
| private highCharts = toSignal(this.highchartsChartService.loaderChanges$); | ||
|
|
||
| private constructorChart = computed<Function>(() => { | ||
| const constructorType = untracked(this.constructorType); |
There was a problem hiding this comment.
If I understand this correctly, it means that changing [constructorType] after the very first initialization will have no effect at all. Is this intentional ? should we be able to change the constructor type while during the component life cycle ? 🤔
If it can indeed never be changed later on, then I think it should be documented as such. WDYT ?
There was a problem hiding this comment.
@PowerKiKi , I kept the same behavior as before. If you check the code in version 4, the ngOnChanges method is just tracking the options and update inputs. That’s why I maintained the same behavior. However, I am open to modifying this to track changes whenever this input changes.
|
@PowerKiKi, thank you for the great feedback and for giving it a try! I have addressed most of your points and decided to include InstanceFactoryFunction as the module factory. Regarding the build issue, I couldn’t reproduce it on my end. Locally, I am using NPM instead of Yarn, so I’m not sure what I might be missing. all these commands : for the library for the demo apps cc @karolkolodziej, I’d love to know if you had a chance to look at my latest commit. Thank you both for the excellent feedback! |
Switched to NPM and do not see compile error anymore 👍 I created a PR for your PR: medbenmakhlouf#1 |
I had the motivation to update the package "highcharts-angular", including the demo app, to the latest version of Angular to take advantage of new reactivity features, now stable as of version 19.
This update will include:
Replacing lifecycle hooks with Signals for better state management.
Introducing Input and Output Signals for component communication.
Utilizing computed and LinkedSignal for derived state.
Applying effect to handle side effects efficiently.
By adopting these changes, we can ensure our components are more reactive, performant, and easier to maintain in the long term, with improved test coverage to support the updates.