Add an ESM build

[joshkel]

Based off of chartjs-plugin-annotation's approach.

Specific changes:

• Switch from Gulp to Rollup for build; using Rollup directly works and seems simpler.
• Refactor some duplicate configuration in rollup.config.js.
• Change import style for Chart.js and Chart.js helpers so that it will work with modules.
• The new import style doesn't allow adding to the imported object, which caused problems for the Chart.Zoom namespace. Since that namespace appears to be undocumented and unused, I removed it.

The ESM version does not automatically register the plugin. This matches chartjs-plugin-annotation and seems to better match Chart.js 3's tree-shaking design.

I'm unclear on whether the UMD build should automatically register the plugin. Not registering seems to better match Chart.js 3, and chartjs-plugin-datalabels recently changed to no longer register, but chartjs-plugin-annotation's UMD build does register.

[benmccann]

Awesome! Thanks for this!

It looks like chartjs-plugin-datalabels was changed to not register so that the plugin isn't automatically enabled on all charts. I agree you may want to enable a plugin only on some charts. I also agree we should probably make sure this plugin, the data labels plugin, and annotations plugin work the same way. @etimberg @kurkle do you have thoughts on UMD vs ESM behavior here and what the standard should be?

[kurkle]

I think we agreed on not registering on any build to be consistent.

[joshkel]

After I asked yesterday about whether plugins should register, I remembered that the v3 migration guide says that Chart.js 3 itself doesn't require registering when it's imported via a <script> tag (which, if I understand correctly, would be when using the UMD build).

So registering in the UMD build seems more consistent?

(I'm happy to do whichever; I just wanted to confirm before proceeding.)

[simonbrunel]

The rationale behind this change in the datalabels plugin is detailed in chartjs/chartjs-plugin-datalabels#42.

So registering in the UMD build seems more consistent?

Auto-registering should have nothing to do with the type of package, especially because UMD builds can be used with require and import (not only <script>). So it's quite inconsistent that the same import syntax against different packages doesn't produce the same result.

And when using <script>, users still have to write JavaScript in order to create a chart, so I don't see any benefit of auto-registering plugins in this case. It brings inconsistencies between import methods, makes more complicated the build/release process and needs to be explained in the docs. All of this to prevent <script> users to write Chart.register(plugin).

[kurkle]

Its not only about plugins though. If we want to be consistent the registration of controllers, elements and scales should be required in all builds too. and that would be silly in my opinion, because you can not shake anything from an umd build. unless we provide some generic registerAll. And if we do that, maybe plugins should do the same?

[simonbrunel]

It's not the same because registering a plugin makes it immediately active while registering a controller / element / scale does not. I think I would actually make registering core plugins required for all builds so it's fully consistent. And again, registering a plugin is not about tree-shaking but about making it "active" for all charts. The fact that there is a single Chart.register() method for all the things may make things a bit confusing.

[simonbrunel]

unless we provide some generic registerAll

I find extremely annoying, not intuitive and not user friendly the way to integrate Chart.js with the ESM build (which is nowadays the most common use case). I understand why "some" wants this granularity of import/register but "most" users should not have to deal with all these imports. For example, a user who wants to create a line chart should not have to think about registering the LineElement, PointElement, LineController, CategoryScale, LinearScale, etc. because these are implicit dependencies coming from defaults.

One solution could be to provide some kind of "presets" so "most" users would just have to:

import { LineChart } from 'chart.js';

Chart.register(LineChart);

new Chart(ctx, {
    type: 'line',
    options,
    data,
});

[benmccann]

There are some such as pie where registration is probably unnecessarily verbose even for tree-shaking users - you can't have a pie / doughnut / polar area chart without radial scale and arc element, so perhaps we should automatically register those for you in that case.

I kind of like the preset idea. LineController is one of the trickiest because it's pretty common not to use the default scales. E.g. the category scale is often switched out for linear or time/timeseries scale. Line/radar is also the only one with multiple possible elements. Registering both elements would make sense for the preset case. Would we register all compatible scales as well? And also all compatible plugins like the filler plugin?

[benmccann]

I think the idea of registering and enabling plugins has gotten a bit muddied. I think plugins should always be registered when imported (i.e. available for use), but should not be globally enabled by default in either the UMD or ESM builds. I think it's reasonable to have an API to globally enable, but we should not call it register as it's an overloaded use of the term and we should call it enable or something else.

[simonbrunel]

I think plugins should always be registered when imported (i.e. available for use)

I disagree on this because it's usually not a good practice to expect side effect on import. Also, we could imagine a plugin that "overrides" another plugin, in which case we don't necessarily want the base plugin to be registered. For example:

import DataLabels from 'chartjs-plugin-datalabels';

// chartjs-plugin-custom-datalabels
export default Object.assign({}, DataLabels , {
   beforeUpdate() {
      // ...
   }
});

I would compare this to Vue.js and their official plugins: they recently chose to remove this side-effect logic in favor of explicit registration. I also think it's a better approach, at least for our plugins. And honestly, I'm not sure to understand what we are trying to simplify?

import ZoomPlugin from 'chartjs-plugin-zoom';

Chart.plugins.register(ZoomPlugin);

vs

import 'chartjs-plugin-zoom';  // side-effect

Chart.plugins.enable('zoom');

The first snippet looks more predictable and robust IMO.

[simonbrunel]

I think plugins should always be registered when imported (i.e. available for use)

@benmccann after reading again your comment, I'm wondering if I correctly understood your suggestion :) i.e. registering the plugin should be a side effect of importing the plugin or if we should expect the user to explicitly call Chart.register() after the import (i.e. the change I made for the datalabels plugin)?

[benmccann]

I was suggesting to have a side effect, but to your point that's not really necessary to avoid verbosity. Maybe a better way to solve my concern without side effects is to rename the API. register to me generally just means to make something available in the registry and I didn't like that it's also having another effect of also globally enabling a plugin. Something like registerAndEnable might be more explicit.

[benmccann]

@joshkel could you rebase this since #416 was merged?

The question about whether to register in the UMD builds seems to be a big one 😄 Perhaps just leave it out for now and we can discuss the best path forward separately without blocking your PR

[jledentu]

I agree with @simonbrunel that the first syntax is much more explicit, and it's a good practice to avoid side effects IMO.

@benmccann Maybe a function named use (like in Vue) would be even more explicit (we don't only "register" something but really "use" it):

Chart.plugins.use(ZoomPlugin);

[kurkle]

@joshkel could you rebase this since #416 was merged?

The question about whether to register in the UMD builds seems to be a big one 😄 Perhaps just leave it out for now and we can discuss the best path forward separately without blocking your PR

I agree to this, the removal of auto-registration needs changes in test setup, so it should be done separately.

[joshkel]

Thanks for the discussion and feedback. I've rebased and responded to the remaining code review comments.

Switching to an explicit use / register step makes sense to me, but as suggested by @kurkle, I preserved the existing behavior (UMD does auto-register) for now, to keep this PR more focused and to leave the tests unchanged.

[benmccann]

removing this will break gulp package so that the release process doesn't work. you might want to leave this but make it run as the first step of package instead of the last step of build

[benmccann]

you should probably leave these and add the esm files as well for packaging. see my other comment below

[joshkel]

If I'm understanding correctly, we don't need these .gitignore lines or the gulpfile copyDistFilesTask.

The previous configuration invoked Rollup to put the files under dist, then it called copyDistFilesTask to copy the files out of dist into the project root (which is why the files are listed in .gitignore here). But package.json references the copies under dist directory, and the gulpfile's packageTask references the copies under dist (via outDir). I can't tell that the copies in the project root are used at all, so I removed them.

If I've missed something, please let me know.

[benmccann]

gulp package looks like it looks for files in the root directory:

chartjs-plugin-zoom/gulpfile.js

Line 64 in 8194ad6

gulp.src([outDir + '*.js', 'LICENSE.md']),

Alternatively, you might be able to update gulp package to look in dist

[joshkel]

If I'm understanding Gulp correctly, it treats outDir as the glob base for outDir + '*.js', so the files from dist are included at the top level of the resulting zip file. Contrast with the explicit base parameter for the samples glob; those are included in a samples subdirectory within the zip file:

chartjs-plugin-zoom/gulpfile.js

Line 68 in 8194ad6

gulp.src('./samples/**/*', {base: '.'})

I tested by running npm run build; npx gulp package in a fresh copy of my branch, and it created a dist/chartjs-plugin-zoom.zip file with contents similar to the releases on GitHub, so I think things are working.

[benmccann]

Ah, we might need to update the comments in the packageTask as they threw me off and look like they might be outdated:

// gather "regular" files landing in the package root
// since we moved the dist files one folder up (package root), we need to rewrite
// samples src="../dist/ to src="../ and then copy them in the /samples directory.

[joshkel]

Yes, I found that confusing too. I just pushed some comments to try and clarify; feedback welcome.

[kurkle]

A lot clearer to me

[benmccann]

Awesome. Thanks for this!!

If you have any interest in contributing further, I'd love to get rid of gulp altogether 😄

[joshkel]

Sure, I can take a look at removing gulp. Thank you.

[benmccann]

@joshkel it's gone now 😄 @kurkle was very fast as making a whole bunch of improvements! thanks again for the help

[kurkle]

Sure, I can take a look at removing gulp. Thank you.

Oh, sorry! I did not see this 😞

[joshkel]

No problem; I hadn't started yet. Thanks, @kurkle and @benmccann.