BREAKING Tree shakable standalone library #1801

PowerKiKi · 2024-01-30T05:10:08Z

The library is standalone and tree-shakable, so it introduces the following breaking changes:

All usage of NgChartsModule must be replaced by provideNgCharts() (and withDefaultRegisterables() and optionally withColorGenerator())

@santam85, this is a draft to transform the library into a pure standalone lib. My goal was to be standalone and tree shakable, hence the introduction of withColorGenerator() (which I'd like to avoid in my own projects).

It is still WIP, because I didn't do much for schematics. I see they are a lot of them to add each possible chart. IMHO that is a bit overkill, and I'd drop support for adding chart entirely. And instead, I'd only keep adding the library to an existing standalone project (not a module project). It is a severe breaking change, but would reduce maintenance drastically. WDYT ?

tl;dr: new usage is like:

provideCharts(withDefaultRegisterables(), withColorGenerator())

The library is standalone and tree-shakable, so it introduces the following breaking changes: - All usage of `NgChartsModule` must be replaced by `provideNgCharts()` (and `withDefaultRegisterables()` and optionally `withColorGenerator()`)

santam85 · 2024-01-30T07:39:30Z

Thanks @PowerKiKi , I agree all the schematics are overkill, I am not opposed to remove the generation ones. As a compromise, we could probably keep a single one with a type parameter to generate a new chart?

IIRC the color generation feature is now something directly supported from Chart.js (https://www.chartjs.org/docs/latest/general/colors.html#default-color-palette) so we could potentially rework that option to include/exclude the extra registrable.

PowerKiKi · 2024-01-30T07:58:51Z

I am not opposed to remove the generation ones

I'll remove all of them then. Because generating hardcoded values does not bring much value, and we already have well documented usages that can be copy-pasted on https://valor-software.com/ng2-charts/

the color generation

I assumed that the hardcoded colors from this lib (which differ from chart.js values), and the generated colors, were something that should be kept. But if that is not the case, and we agree that colors will change in this release, then I agree it would make more sense to drop it entirely and instead recommend something like https://github.com/kurkle/chartjs-plugin-autocolors. Would that be ok with you ?

santam85 · 2024-01-30T09:11:46Z

Rather than suggesting en external library ('autocolor' from https://github.com/kurkle/chartjs-plugin-autocolors) I would prefer using the built-in one already present in Chart.js.

It should be possible to test it in our lib by adding these defaults:

import { Colors } from 'chart.js';

Chart.register(Colors);

and for the charts:

  plugins: {
    colors: {
      enabled: true
    }
  }

PowerKiKi · 2024-01-30T11:44:18Z

Actually the built-in Colors plugin will "will cycle through a palette of seven Chart.js brand colors". To double check that statement I made a StackBlitz with pure Chart.js with 14 datasets, and clearly colors are not auto-generated but instead cycled:

So what should we do ? drop and recommend that [autocolors](https://github.com/kurkle/chartjs-plugin-autocolors) plugin ? drop and don't recommend anything ? keep something in our lib ?

santam85 · 2024-01-30T11:58:38Z

My opinion is we should:

drop color generation (we start with a palette of 12 before generating too)
default to registering and enabling Colors from Chart.js as it does not require any extra dependency, state in the release notes that the palette only has 7 colors
Point people in need of more colors to the autocolor plugin if they need color generation, but avoid adding it to our dependencies, add a sample in the docs app on how to configure it

This can be part of a separate PR too though...

Instead you should either create components manually or use the directive directly in one of your existing components.

PowerKiKi · 2024-01-30T12:40:53Z

This is ready for review (and merge). I'll address the color thing in a separate PR.

santam85

Nice work, thanks for the help!
Can you take care of updating the changelog as well for the library please? That way we can start tracking the breaking changes for 6.0.0

santam85 · 2024-01-31T08:58:54Z

libs/ng2-charts/README.md

-// In your App's module:
-imports: [NgChartsModule];
+@Component({
+  standalone: true,


It's not mandatory to use the directive in a standalone component right? Should we make that explicit?

Documented in 7b0bfa8

santam85 · 2024-01-31T09:00:16Z

libs/ng2-charts/src/lib/get-colors.ts


-export const builtInDefaults = {
+export const builtInDefaults :AnyObject= {


Isn't there a more restrictive type we could use for defaults in Chart.js?

I wish, but unfortunately Chart.js typing is quite loose for that method:

https://github.com/chartjs/Chart.js/blob/767d64e7a90dbfe94f9c5d159a890054818c1680/src/types/index.d.ts#L673

santam85 · 2024-01-31T09:02:26Z

README.md

+
+bootstrapApplication(AppComponent, {
+  providers: [
+    provideCharts(withDefaultRegisterables(), withColorGenerator()),


Can you please also add a config example with a custom registerables list as you did in the docs app?

Done in 0d894e5

santam85 · 2024-01-31T13:54:20Z

fixes #1771 , #1360

PowerKiKi · 2024-02-01T00:55:29Z

I did everything you asked, except updating CHANGELOG.md, because that file was not kept up to for 4.0.1, 4.0.2, 4.1.0, 4.1.1, 5.0.0, 5.0.1, 5.0.2, 5.0.3 and 5.0.4. I'd rather not do that 😅. Instead I would suggest to delete CHANGELOG.md and only use GitHub releases, which are up-to-date and easily generated. I know it's slightly less convenient for users, but CHANGELOG.md maintenance is a pain. And i'd rather have a systematically up-to-date non-file changelog in GitHub, rather than incomplete CHANGELOG.md file.

Because that file was not kept up to for 4.0.1, 4.0.2, 4.1.0, 4.1.1, 5.0 .0, 5.0.1, 5.0.2, 5.0.3 and 5.0.4. Instead, we will keep using GitHub auto-generated release notes. As discussed in valor-software#1801 (comment)

PowerKiKi · 2024-02-01T10:03:42Z

IMHO this is ready to be merged. Would you like me to rebase/squash ?

Because that file was not kept up to for 4.0.1, 4.0.2, 4.1.0, 4.1.1, 5.0 .0, 5.0.1, 5.0.2, 5.0.3 and 5.0.4. Instead, we will keep using GitHub auto-generated release notes. As discussed in #1801 (comment)

`builtInDefaults` and `baseColors` were dropped. That means that colors will change to be the default as defined by Chart.js, and we will no longer generate colors on the fly. If you need this kind of features, see https://www.chartjs.org/docs/latest/general/colors.html#advanced-color-palettes As discussed in valor-software#1801 (comment)

`builtInDefaults` and `baseColors` were dropped. That means that colors will change to be the default as defined by Chart.js, and we will no longer generate colors on the fly. If you need this kind of features, see https://www.chartjs.org/docs/latest/general/colors.html#advanced-color-palettes As discussed in #1801 (comment)

PowerKiKi force-pushed the powerkiki branch from e4c0f95 to da5b2b3 Compare January 30, 2024 07:29

BREAKING Tree shakable standalone library

2f2a289

The library is standalone and tree-shakable, so it introduces the following breaking changes: - All usage of `NgChartsModule` must be replaced by `provideNgCharts()` (and `withDefaultRegisterables()` and optionally `withColorGenerator()`)

PowerKiKi force-pushed the powerkiki branch from da5b2b3 to 2f2a289 Compare January 30, 2024 07:29

PowerKiKi added 2 commits January 30, 2024 20:04

Drop all schematics to generate chart component

fd1b82f

Instead you should either create components manually or use the directive directly in one of your existing components.

Support (only) standalone app to add the library to it

e400a9d

PowerKiKi force-pushed the powerkiki branch from f63907c to e400a9d Compare January 30, 2024 12:37

PowerKiKi marked this pull request as ready for review January 30, 2024 12:40

santam85 requested changes Jan 31, 2024

View reviewed changes

PowerKiKi added 3 commits February 1, 2024 08:19

Rename file according to its content

ba872a0

Document usage with modules

7b0bfa8

Document usage with tree shaking

0d894e5

PowerKiKi mentioned this pull request Feb 1, 2024

Enforce prettier in CI #1804

Merged

PowerKiKi mentioned this pull request Feb 1, 2024

Drop CHANGELOG.md #1805

Merged

Import correct path

15a5a2d

santam85 merged commit 1353be6 into valor-software:master Feb 1, 2024
1 check passed

PowerKiKi mentioned this pull request Feb 1, 2024

BREAKING drop hardcoded and generated colors #1806

Merged

crodriguez6497 mentioned this pull request Mar 4, 2024

Module '"ng2-charts"' has no exported member in v6 #1864

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BREAKING Tree shakable standalone library #1801

BREAKING Tree shakable standalone library #1801

PowerKiKi commented Jan 30, 2024 •

edited

santam85 commented Jan 30, 2024 •

edited

PowerKiKi commented Jan 30, 2024 •

edited

santam85 commented Jan 30, 2024

PowerKiKi commented Jan 30, 2024

santam85 commented Jan 30, 2024 •

edited

PowerKiKi commented Jan 30, 2024

santam85 left a comment

santam85 Jan 31, 2024

PowerKiKi Feb 1, 2024

santam85 Jan 31, 2024

PowerKiKi Feb 1, 2024

santam85 Jan 31, 2024

PowerKiKi Feb 1, 2024

santam85 commented Jan 31, 2024 •

edited

PowerKiKi commented Feb 1, 2024

PowerKiKi commented Feb 1, 2024


		export const builtInDefaults = {
		export const builtInDefaults :AnyObject= {

BREAKING Tree shakable standalone library #1801

BREAKING Tree shakable standalone library #1801

Conversation

PowerKiKi commented Jan 30, 2024 • edited

santam85 commented Jan 30, 2024 • edited

PowerKiKi commented Jan 30, 2024 • edited

santam85 commented Jan 30, 2024

PowerKiKi commented Jan 30, 2024

santam85 commented Jan 30, 2024 • edited

PowerKiKi commented Jan 30, 2024

santam85 left a comment

Choose a reason for hiding this comment

santam85 Jan 31, 2024

Choose a reason for hiding this comment

PowerKiKi Feb 1, 2024

Choose a reason for hiding this comment

santam85 Jan 31, 2024

Choose a reason for hiding this comment

PowerKiKi Feb 1, 2024

Choose a reason for hiding this comment

santam85 Jan 31, 2024

Choose a reason for hiding this comment

PowerKiKi Feb 1, 2024

Choose a reason for hiding this comment

santam85 commented Jan 31, 2024 • edited

PowerKiKi commented Feb 1, 2024

PowerKiKi commented Feb 1, 2024

PowerKiKi commented Jan 30, 2024 •

edited

santam85 commented Jan 30, 2024 •

edited

PowerKiKi commented Jan 30, 2024 •

edited

santam85 commented Jan 30, 2024 •

edited

santam85 commented Jan 31, 2024 •

edited