v3: consider changing export logic for esm modules

[sgratzl]

Feature Proposal

the in the v3 version the enhanced Chart module is exported as the solely (default) export from the library.

https://github.com/chartjs/Chart.js/blob/master/src/index.js

As an alternative one could have the following structure:

import ChartChart from './core/core.controller';
export { default as Chart } from './core/core.controller';
import DatasetController from './core/core.datasetController';
export { default as DatasetController }  from './core/core.datasetController';

class Chart extends ChartChart {
}
Chart.DatasetController = DatasetController;

export default Chart;

the purpose around the Chart class is that only the default export has all the dependencies but not the class itself.

Feature Use Case

I want to prepare for tree shaking and thus only import things that I really need. e.g.,

import {DatasetController} from 'chart.js';

class MyController extends DatasetController {
..}

note: while this works already with some synthetic imports (by rollup, ...) it is not standard conform.

Possible Implementation

[benmccann]

I would love to make Chart.js tree-shakeable

We had investigated a bit earlier. Some notes:

• We'd probably want two separate entry points. One for the UMD bundle that we create that provides everything and one for the ES6 build consumed via npm that is tree-shakeable
• @kurkle and I had a different understanding of how the bundlers did tree shaking. We did some experimentation and although my memory is a little fuzzy, I think we came to the following conclusion: webpack does tree-shaking on a per-file basis unless sideEffects: false is set.
  • Chart.js does have side-effects right now because the scales get automatically registered
• The biggest debate we've had is what the API should be.
  • Right now you specify scales and controllers with strings. E.g. 'time' or 'line'. I believe @etimberg suggested we keep it this way
  • Assuming we continue with the string API, it means we need to continue registering scales and also introduce registration for controllers.
  • Should scales and controllers register themselves when imported (Have scales and plugins register themselves #6979) or should we make users explicitly register them? While it's probably easier to have them auto-registered, that would reduce the effectiveness of treeshaking since we couldn't set sideEffects: false. I think deciding on this last point is the only real thing we need to do before implementing tree shaking.
    • We were previously blocked by a custom rollup css plugin that has since been removed, so it should be relatively doable now

@sgratzl any chance you would want to implement this? :-) We're all pretty busy and haven't had much time for core development lately, but we could talk and decide that last point if someone has the time to implement it

[kurkle]

Some quick opinions:

• I'd most likely go with manual registration after importing (of scales and controllers)
• I'd call the it Core or ChartCore or CoreChart rather than ChartChart.

[etimberg]

I am in favour of the string API if possible. I think its a better default out of the box since it's backwards compatible with v2 code and also quite easy to understand. It also doesn't break any existing documentation that's out there on SO or equivalent.

If we can do an opt-in tree shaking API initially, I am on-board. That would let us experiment and see what works well and then consider how to migrate all users to it eventually.

[benmccann]

Ok, I think basically all we need to do is:

• Register controllers just like we do scales and use the registry here
  • It would be good to create a generic register method that can take scales and controllers so users can register everything they need in one line
• Create a new entry point for npm / esm users

[sgratzl]

I think the biggest side effect in the library are the defaults. e.g., while elements don't need to be registered their defaults need to be, Thus creating a side effect. One approach could be using the same idea as in the scale case ->static .defaults attribute.

in https://github.com/sgratzl/chartjs-chart-boxplot/tree/develop I adapted the plugin to support tree shaking as good as possible. Essential changes:

• controller, elements, scale all have .id, .defaults. and .register. The .register function registers it within chart.js creating side effects. Moreover, when registering a controller, it also registers all its default elements and scales.
• I created https://github.com/sgratzl/chartjs-chart-boxplot/blob/develop/src/chart.js which is a wrapper around the library with a flat export hierarchy and utility register functions. In the best case chart.js at some point looks like this.
• For each controller I created a class inheriting the Chart chart which injects the type into its config. For example, the BoxPlotChart can be used to create a boxplot with implicitly registered the BoxPlotController. see also https://github.com/sgratzl/chartjs-chart-boxplot/blob/develop/samples/default_esm.html

[sgratzl]

* It would be good to create a generic register method that can take scales and controllers so users can register everything they need in one line

I would prefer a function per type. it is easier to ensure that the right arguments are given and one can avoid conflicts in names. e.g. a Violin controller and a Violin element.

* Create a new entry point for npm / esm users

I have a bundle.js and index.js where the bundle.js is like the esm index.js but registers everything for the UMD case.

I am in favour of the string API if possible. I think its a better default out of the box since it's backwards compatible with v2 code and also quite easy to understand. It also doesn't break any existing documentation that's out there on SO or equivalent.

in the best case, chart.js supports two options everywhere where a .type is required:

1. the string id that will be looked up in the registry
2. a constructor / function that is used to create the instance directly.

relevant locations as far as I can see:

• scale type, e.g. scales.x.type = 'category'
• controller type, e.g., .type = 'bar'
• element type, e.g., BarController.prototype.dataElementType = Rectangle
• interpolator type, e.,g., `animation.numbers.type = 'number'

[benmccann]

I would prefer a function per type. it is easier to ensure that the right arguments are given and one can avoid conflicts in names. e.g. a Violin controller and a Violin element.

If we had one method it could check the type (e.g. is it a subclass of DatasetController or Scale) and still store it in a separate registry based on type

I have a bundle.js and index.js where the bundle.js is like the esm index.js but registers everything for the UMD case.

That sounds good to me

in the best case, chart.js supports two options everywhere where a .type is required:
the string id that will be looked up in the registry
a constructor / function that is used to create the instance directly.

We actually only need the constructor. At least that's how scale registration works now. The id is a static property on the constructor

in the best case, chart.js supports two options everywhere where a .type is required

We had discussed this as an option earlier, but I think the consensus was to only support the string option via registration in v3 because we were worried it would be too confusing to have two ways to do things and would confuse people when they were looking at samples on StackOverflow, etc.

[sgratzl]

I played a bit with tree shaking by manually commenting out things from the build that are not needed for a bar chart and then use rollup to remove all the things that fall of. see https://github.com/sgratzl/chartjs-treeshake-test.

in summary:

• ESM build of charts.js full: 348 kB
• ESM build of chart.js for barchart only: 249 kB

it could be a bit less if the helpers object is not used within the code but just the methods directly. Then, some parts of the helpers object could be shaken out, e.g., in this example splineCurve and so on.

besides removing all the wrappers (elements, plugins, scales, ...) I mostly had to comment out access of the prototypes (e.g., LineController.prototype.dataElementOptions = ...) and manipulating the defaults object, (e..g, defaults.set('line', ....). The former prevents the LineController from being shaken since it is actually used and the later since its config is not needed.

[benmccann]

Wow, that's great! I'm a little surprised that not more was able to be shaken out, but it's a good start. No one will complain about a 30% reduction in build size! If you want to send us any PRs to make these changes we'd be happy to review. (btw, I didn't understand what you meant about wrappers)

Code tends to shake out better when it's functions. We use a lot of classes and those don't shake out well, so I guess that's the core issue. I think there are some additional wins we can get by updating things a bit.

E.g. one I noticed is that the whole color library is getting pulled in and we don't need most of it. We really just use mix in core.animation. It's class-based like the rest of our code and that class depends on most of the other files in the project. However, if we updated it to be just a collection of functions like date-fns then we could shake most of it out. That might require some changes to our API. E.g. we have code to parse any type of color that a user gives to us right now. It might be better to take only rgba and then if a user wants to provide something like hsl, they can import the color library and do the parsing themselves. That would save it from being imported into the build for every user regardless of whether they use that functionality.

Another thing I noticed is that your build had the ResizeObserver polyfill in it. That shouldn't be present in our ES6 builds (we'd expect users to provide their own polyfill if they want to support older browsers), so hopefully we could find a way to remove it as well

[sgratzl]

Wow, that's great! I'm a little surprised that not more was able to be shaken out, but it's a good start. No one will complain about a 30% reduction in build size! If you want to send us any PRs to make these changes we'd be happy to review. (btw, I didn't understand what you meant about wrappers)

Code tends to shake out better when it's functions. We use a lot of classes and those don't shake out well, so I guess that's the core issue. I think there are some additional wins we can get by updating things a bit.

the main question I wonder is how to deal with detault values. e.g. all the defaults.set(...) calls that are not bound to the class and thus not shaken out. Moreover, in two or three locations (legend, title, ...) the defaults.elements.line are used, even tho I don't know how those are related to the context.

E.g. one I noticed is that the whole color library is getting pulled in and we don't need most of it. We really just use mix in core.animation. It's class-based like the rest of our code and that class depends on most of the other files in the project. However, if we updated it to be just a collection of functions like date-fns then we could shake most of it out. That might require some changes to our API. E.g. we have code to parse any type of color that a user gives to us right now. It might be better to take only rgba and then if a user wants to provide something like hsl, they can import the color library and do the parsing themselves. That would save it from being imported into the build for every user regardless of whether they use that functionality.

one could consider switching to a different library such as d3-color or tinycolor. regarding custom parsing, it would require that uses can manipulate the interpolators or the automatic hover color.

Another thing I noticed is that your build had the ResizeObserver polyfill in it. That shouldn't be present in our ES6 builds (we'd expect users to provide their own polyfill if they want to support older browsers), so hopefully we could find a way to remove it as well

if you take a look at https://unpkg.com/[email protected]/dist/Chart.esm.js?module it is part of your esm build. maybe babel injects it because of your browserlist settings.

[kurkle]

if you take a look at https://unpkg.com/[email protected]/dist/Chart.esm.js?module it is part of your esm build. maybe babel injects it because of your browserlist settings.

https://www.chartjs.org/dist/master/Chart.esm.js does not contain it anymore.

[kurkle]

the main question I wonder is how to deal with detault values. e.g. all the defaults.set(...) calls that are >not bound to the class and thus not shaken out.

Maybe apply the defaults when registering the controller / element. Part of the class prototype? Not sure where the best location to ship those is.

[benmccann]

Moreover, in two or three locations (legend, title, ...) the defaults.elements.line are used, even tho I don't know how those are related to the context.

It looks like just the legend has that issue. We had at one point talked about removing the element options. Regardless though, I would be open to giving the legend its own defaults instead of using defaults.elements.line. As a temporary workaround we could just import the LineElement into the entry point to ensure those defaults are available and unblock the rest of the tree shaking

Maybe apply the defaults when registering the controller / element.

That sounds good to me.

Part of the class prototype?

@sgratzl had said earlier "I mostly had to comment out access of the prototypes (e.g., LineController.prototype.dataElementOptions = ...). [They prevent the] LineController from being shaken since it is actually used". I'm not sure if there's a better way to do this. Does it work if we make them static fields?

[sebiniemann]

I have seen that most imports are leaving out the file extension.

While this is fine for webpack/rollup or any other bundler that tries out some extension etc. in their resolver logic, this won't work for direct node or browser imports.

See https://nodejs.org/api/esm.html#esm_mandatory_file_extensions, "Mandatory file extensions".

[kurkle]

I have seen that most imports are leaving out the file extension.

While this is fine for webpack/rollup or any other bundler that tries out some extension etc. in their resolver logic, this won't work for direct node or browser imports.

See https://nodejs.org/api/esm.html#esm_mandatory_file_extensions, "Mandatory file extensions".

I'm not sure why this is relevant, we are using rollup and babel to transpile the stuff before publishing it to npm.

[sgratzl]

one general point which there needs to be a consensus about before continuing:

There are two common ways how to deal with tree-shaking:

1. flat: export everything as a single index.js esm file which has a flat hierarchy and allow tools like rollup/webpack to remove functions/classes that were not imported. Code wise it means that the index.js file just contains of re exports from other modules (e.g., export * from or export {default as ... and rollup creates a single esm file containing everything.

2. separated: have multiple files which needs to be imported. Thus, following a import what you want approach. So, instead of a remove what you don't need to add what you need. It requires that the build is not just one file but all the files that should be importable. e.g., import BarController from 'chart.js/controllers/bar requires that the npm package has a file: controllers/bar.js.

The separted case can be extended to also support the flat case but requires to have the sideEffects false flag to be set.

pros/cons

• pro flat: fits the current build system
• con separated: the typescript typings need also provide the same files for proper typings
• con separated: individual files need optionally to be transpiled to avoid higher babel features in the build (like static properties) and probably some renaming e.g. src/controllers/controller.bar.js to controllers/bar.js
• pro separated: no renaming required since the import path can reflect its meaning, e.g. import {clear} from 'chart.js/helpers/canvas
• ?

[kurkle]

@sgratzl take a look at #7400, what do you think about that one?
Everything else is flat, except helpers.

For import {isObject} from 'chart.js/helpers/core'; to work, I moved the outputs to root (from dist).
If not moved, it would need to be import {isObject} from 'chart.js/dist/helpers/core';

[kurkle]

@sgratzl had said earlier "I mostly had to comment out access of the prototypes (e.g., LineController.prototype.dataElementOptions = ...). [They prevent the] LineController from being shaken since it is actually used". I'm not sure if there's a better way to do this. Does it work if we make them static fields?

Maybe those (dataElementOptions / datasetElementOptions) should also be part of the registration process.

Edit: The static fields are just some syntax sugar afaik, so those won't really help.

[sebiniemann]

I wouldn't say that you have to choose between either a single bundle file or direct delivery of the source files, but you could do both. I'm not sure how common this is yet, but I see it with most of the modules we work with (VueJS, Vuetify, ...).

Taking a note from #7400

no idea really. could be referenced directly from browser, i think?

Importing the source files directly from a CDN via <script type="module" src="chartjs.js"></script> (where chartjs.js is not a bundle, but imports other needed files itself) can also make use of caching and will only reload changed files, which would not be possible with a bundle. It also eases bug reports, as we would got the same file/line reported in user land as in this repo (not needing any mapping files anymore).

I wouldn't say that a (single-file) bundle is an old artifact, now that JS is capable of module-support, but I see its main use only in supporting old browsers that are incapable of type="module" as well as older NodeJS versions (before v13.2) where ESM support was still experimental.

In addition, users who use their own Webpack/Rollup/... bundle-process have also no need for a pre-bundled version, regardless which NodeJS version they are using or which browsers they want to support.

---

In summary, it would be great if both could be supported (adding the source folder to the npm package and using the full file name in the imports).

[sgratzl]

what about splitting up this issue, this one contains the discussion about how to structure the ESM build and another one about how to do the registration process for different components: controller, scales, elements, plugins

[kurkle]

@sgratzl ready to close this one?

[sgratzl]

it would be nice if the logic and methods of this code block is generalized and also available in the ESM build:

Chart.js/src/index.js

Lines 28 to 38 in 426d8de

	// @ts-ignore
	const invalidatePlugins = () => each(Chart.instances, (chart) => chart._plugins.invalidate());
	Chart.register = (...items) => {
	registry.add(...items);
	invalidatePlugins();
	};
	Chart.unregister = (...items) => {
	registry.remove(...items);
	invalidatePlugins();
	};

[sneko]

Hi 😃 !

Just curious, do you have an approximate release "date/month" for tree-shaking? (I guess it's on your spare time, but just curious if any).

Thank you,

[benmccann]

any day now

[etimberg]

Yes, we're quite close. Barring any last minute issues, we'll probably do the v3.0.0-alpha.2 release tonight or tomorrow

[jonrimmer]

Separating the helpers in the ESM build has broken chartjs-adapter-luxon and chartjs-adapter-datefns because they expect helpers to be a property on the chart.js export: https://github.com/chartjs/chartjs-adapter-luxon/blob/master/src/index.js#L34

[benmccann]

I've sent fixes for the date adapters:
chartjs/chartjs-adapter-luxon#20
chartjs/chartjs-adapter-date-fns#16

In the meantime, you can avoid any issues by supplying timestamps and setting parsing: false so that the parse method in the date adapters is not called

[kurkle]

@sgratzl are there any more changes, you'd like to the export logic of v3?

[sgratzl]

@sgratzl are there any more changes, you'd like to the export logic of v3?

no, looking great, thx to all for all the effort