Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Propose a simplified registration process #8419

Closed

Conversation

etimberg
Copy link
Member

@etimberg etimberg commented Feb 13, 2021

Background

Starting in V3, we moved ESM builds to require registration of elements, controllers, plugins, and scales. This enables bundlers such as webpack to remove unused code from the build. However, forcing users to deal with this problem caused a number of headaches.

Proposal

This PR separates tree-shaking into a separate sub-package, chart.js/tree. When users import from this location, they are required to register the elements they are using. If the user imports from chart.js, they can use a default import that has all of the necessary pieces pre-registered.

Testing

I conducted local testing using a separate app available at https://github.com/etimberg/chartjs-tree-shaking-test
I tested imports from both chart.js and chart.js/tree. I confirmed that tree-shaking took place when using the chart.js/tree package + registration.

@etimberg
Copy link
Member Author

I tested this out locally with https://github.com/etimberg/chartjs-tree-shaking-test and was able to see the benefits of tree-shaking when using the chart.js/tree package.

ESM imports of `chart.js` will no longer require registration of
controllers, scales, elements, or plugins. Instead, everything is
pre-registered.

To use tree shaking, imports will need to be made from the
'chart.js/tree' package and will require registration.
@etimberg etimberg force-pushed the export-simpler-registration-methods branch from 36d2a94 to f5b6b31 Compare February 13, 2021 19:46
Copy link
Member

@simonbrunel simonbrunel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @etimberg for looking at this :)

I think I would keep the regular chart.js import shakable and only provide shortcuts to register all the things:

import {
   Chart,
   controllers,
   elements,
   plugins,
   scales,
} from 'chart.js';

Chart.register(
    ...Object.values(controllers),
    ...Object.values(elements),
    ...Object.values(plugins),
    ...Object.values(scales),
);

// or

import { Chart, registrables } from 'chart.js';

Chart.register(...registrables);

I'm not really sure we need the auto-register (none shakable) version because users could directly use the UMD build instead. In the case of the datalabels docs, I can definitely live with one version above, else I would use the UMD build. If you think we should provide an ESM build for this use case (auto-register), then I would make it the special case (i.e. import 'chart.js/auto' or whatever name).

A short registration format is also available.

```javascript
import { Chart, controllers, elements, plugins, scales } from 'chart.js';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really working? I can't find were you export these groups.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I forgot to delete that part of the docs when I switched around how this worked.

@@ -30,7 +30,9 @@
"dist/chunks/*.js",
"dist/chunks/*.d.ts",
"helpers/**/*.js",
"helpers/**/*.d.ts"
"helpers/**/*.d.ts",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of the scope of this PR: could this be moved under a single folder and copied when publishing to npm? For example: ./esm/helpers, ./esm/tree, etc. (or a different folder name).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think NPM in that case might consider the package to be chart.js/dist/helpers but we'd have to experiment and see

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These extra packages could even be generated by a script as we did with bowser.json


### Tree Shaking

Chart.js 3 has optional tree shaking that is enabled via the `chart.js/tree` package. To use this, it is necessary to import and register the controllers, elements, scales and plugins you wish to use. The example below shows all of the available imports and how to register them.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think users expect the default behavior (i.e. import {...} from 'chart.js') to be tree-shakable, which may be the most common use case. It's also the natural method to import from Chart.js and it may be better to keep it that way. As a plugin developers, I would also expect the following snippet to only bring LineElement and not trigger any auto-registration:

import { LineElement } from 'chart.js';

export class CurveElement extends LineElement {
  // ...
}

It may be better to have a special case for auto-registering instead (e.g. chart.js/auto or chart.js/all).


```javascript
import { Chart, controllers, elements, plugins, scales } from 'chart.js';
Chart.register(...[
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need to wrap everything in an array:

Chart.register(
    ...Object.values(controllers),
    ...Object.values(elements),
    ...Object.values(plugins),
    ...Object.values(scales),
);

...Object.values(elements),
...Object.values(plugins),
...Object.values(scales),
]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could make this even easier by exporting an array containing everything:

import { registrables } from 'chart.js';

Chart.register(...registrables);

You might have a better name for registrables.

export * from './elements';
export * from './platform';
export * from './plugins';
export * from './scales';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You still need to export those groups:

// ...
export * from './plugins';
export * from './scales';

// Exporting groups to simplify the registration.
export * as controllers from './controllers';
export * as elements from './elements';
export * as plugins from './plugins';
export * as scales from './scales';

@etimberg
Copy link
Member Author

etimberg commented Feb 14, 2021

So I tested the single package version locally and it works for tree shaking as well.

index.esm.js

export * from './controllers';
export * from './core';
export * from './elements';
export * from './platform';
export * from './plugins';
export * from './scales';

export * as controllers from './controllers';
export * as elements from './elements';
export * as plugins from './plugins';
export * as scales from './scales';

In the test project, to register all

import Chart, { controllers, elements, plugins, scales } from 'chart.js';
Chart.register(controllers, elements, plugins, scales);

In the test project to register some

import {
  Chart,
  LineElement,
  PointElement,
  LineController,
  CategoryScale,
  LinearScale,
  Legend,
  Tooltip
} from 'chart.js';

Chart.register(
  LineElement, PointElement, LineController,
  CategoryScale, LinearScale,
  Legend, Tooltip,
)

The outcome of this is the same as before. 164kB for the all version and 155kB for the tree-shaken version.

@kurkle since the separate package was your idea, any opinions?

Edit:
I also tested the registerables version

export const registerables = [
  ...Object.values(controllers),
  ...Object.values(elements),
  ...Object.values(plugins),
  ...Object.values(scales),
];

It did not tree-shake correctly because the registerables array was not marked as /*#__PURE__*/ by webpack.

If I change the export to

export const registerables = [
  controllers,
  elements,
  plugins,
  scales,
];

it works, but now the user must do Chart.register(...registerables).

@simonbrunel
Copy link
Member

It did not tree-shake correctly because the registerables array was not marked as /#PURE/ by webpack.

I think it works if you don't use Object.values() but instead create a src/registrables.js file exporting each import:

// src/registrables.js`
import { BarController, LineController, ... } from './controllers';
import { LineElement, PointElement,, ... } from './elements';
// ... all other registrables imports

export const registrables: [
  BarController,
  LineController,
  LineElement,
  PointElement,
  // ...
];
// src/index.esm.js
export * from './registrables.js';

@etimberg
Copy link
Member Author

Yup, removing Object.values should fix it. Locally I preferred the array containing the maps directly, i.e. registerables = [controllers, elements, plugins, scales]; so that it leaves one less place to modify if we ever add a new plugin, controller, etc.

@simonbrunel
Copy link
Member

simonbrunel commented Feb 14, 2021

Though, I'm not against the separate package, however I don't think the chart.js import should auto-register everything (as explained in a previous comment). If we decide to add this separate package, I would still export groups of imports (i.e. controller, elements, etc.) but not the registrables array which may be useless in this case.

@etimberg
Copy link
Member Author

Now that I have proof the single package works, it's probably easier since there's less to maintain and the diff is a lot smaller. So I'd propose we go back to requiring registration, export groups & a single registerables list to provide some flexibility. I put up those changes in #8425

@kurkle
Copy link
Member

kurkle commented Feb 14, 2021

just commenting the registables, I think this should work:
export Object.assign({}, controllers, scales, elements, plugins)

@etimberg
Copy link
Member Author

Closing in favour of #8425

@etimberg etimberg closed this Feb 14, 2021
@etimberg etimberg deleted the export-simpler-registration-methods branch March 21, 2021 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants