Make moment optional from our UMD builds

[simonbrunel]

Create a rollup plugin altering the UMD header to wrap optional dependencies between try/catch, which allows to load moment only when the dependency is installed.

Since AMD loaders are asynchronous, 'moment' needs to be explicitly loaded before 'chart.js' so when 'chart.js' requires moment, it's already loaded and returns synchronously (at least with requirejs).

require(['moment'], function() {
    require(['chartjs'], function(Chart) {
        new Chart('chart', {
            //...

Still need to write a bit of docs and do more testing.

UMD wrapper in dist/Chart.js:

Before:

(function (global, factory) {
    typeof exports === 'object' && typeof module !== 'undefined' ? module.exports = factory(require('moment')) :
    typeof define === 'function' && define.amd ? define(['moment'], factory) :
    (global.Chart = factory(global.moment));
}(this, (function (moment) { 'use strict';

After:

(function (global, factory) {
    typeof exports === 'object' && typeof module !== 'undefined' ? module.exports = factory(function() { try { return require('moment'); } catch(e) { } }()) :
    typeof define === 'function' && define.amd ? define(['require'], function(require) { return factory(function() { try { return require('moment'); } catch(e) { } }()); }) :
    (global.Chart = factory(global.moment));
}(this, (function (moment) { 'use strict';

Related to #5960
Fixes #4303

[benmccann]

docs/getting-started/integration.md would be a good place to mention:

• webpack: you'll get a warning Module not found: Error: Can't resolve 'moment' that can be ignored if you don't include moment
• including moment before chartjs:

 require(['moment'], function() {
    require(['chartjs'], function(Chart) {
        new Chart('chart', {

Also, we'll need to move moment from dependencies to peerDependencies and devDependencies to make it optional. Otherwise users won't have any choice and will have it included automatically

[simonbrunel]

Thanks @benmccann, will update the docs with your suggestions.

we'll need to move moment from dependencies to peerDependencies and devDependencies to make it optional. Otherwise users won't have any choice and will have it included automatically.

I agree, however we need to figure out how to prevent breaking existing integrations (as explained in this comment). The problem appears when a npm project installed chart.js only (no explicit moment dependency) and then require('chart.js') or import('chart.js'). In that case, moment installed as part of our package.json dependency is bundled in their builds and so they can use the time scale.

If moment becomes a peer dependency, these projects will still build and deploy successfully (despite there is a warning message) but without moment, thus the time scale will not work anymore. IMO, that's a breaking change not acceptable for a minor release.

[benmccann]

How about adding something like this in docs/getting-started/installation.md then:

If you would like to install chart.js without moment.js, either because you are not using the time scale or because you would like to use an alternate date adapter, you can setup npm to automatically remove the moment dependency postinstall by updating your package.json:

  "scripts": {
    "postinstall": "rm -rf ./node_modules/moment"
  },

[benmccann]

Per convo on slack, let's document for users how to ignore moment in bundlers:

Maybe https://webpack.js.org/configuration/externals/ will work for webpack.

Same for rollup: https://github.com/chartjs/Chart.js/blob/master/rollup.config.js#L37

[benmccann]

I just tested the below config with Webpack and it does indeed appear to work:

  externals: {
    moment: 'moment'
  },

[simonbrunel]

Thanks for testing, will update that PR as soon as I'm back at home next week.

[simonbrunel]

Tested again with Require.js and I'm no sure we should override the AMD wrapper:

(A) AMD without optional dependency

the rollup built-in wrapper

// User wants to use the time scale
requirejs.config({
  paths: {
    'moment': 'path/to/moment',
    'chartjs': 'path/to/Chart'
  }
});

// [OK] moment is loaded as a dependency of chartjs, time scale "enabled"
require(['chartjs'], function(Chart) {});

// User DON'T want to use the time scale
requirejs.config({
  paths: {
    'chartjs': 'path/to/Chart'
  }
});

// [FAIL] moment is not found, requirejs throws an UNCATCHABLE exception
require(['chartjs'], function(Chart) {});

To make moment optional, the user would need a fake 'moment':

// User DON'T want to use the time scale
requirejs.config({
  paths: {
    'chartjs': 'path/to/Chart'
  }
});

// Fake 'moment' module
define('moment', function() {});

// [OK] moment is "undefined", no requirejs exception, time scale "disabled"
require(['chartjs'], function(Chart) {});

(B) AMD with optional dependency

the modified wrapper (see this line)

Note: we are overriding the AMD loader to make the moment require synchronous, which means that the user will have to explicitly require moment before Chart.js.

// User wants to use the time scale
requirejs.config({
  paths: {
    'moment': 'path/to/moment',
    'chartjs': 'path/to/Chart'
  }
});

// [FAIL] moment, as a dependency of chartjs, is not yet loaded, time scale "disabled"
require(['chartjs'], function(Chart) {});

// [FAIL]
require(['moment', 'chartjs'], function(Chart) {});

// User wants to use the time scale
requirejs.config({
  paths: {
    'moment': 'path/to/moment',
    'chartjs': 'path/to/Chart'
  }
});

// [OK] moment, as a dependency of chartjs, is loaded, time scale "enabled"
require(['moment'], function() {
    require(['chartjs'], function(Chart) {});
});

// User DON'T want to use the time scale
requirejs.config({
  paths: {
    'chartjs': 'path/to/Chart'
  }
});

// [OK] moment is not found, no requirejs exception, time scale "disabled"
require(['chartjs'], function(Chart) {});

[simonbrunel]

The main issue with (B) is the asynchronous nature of AMD loaders, which doesn't allow us to catch the exception when 'moment' is not found. This PR makes this require synchronous but works only if moment is already loaded. If we could catch the asynchronous exception, then the first snippet of (B) would work and this solution would be perfect.

So basically, we have to choose between two situations for which we ask the user to use workarounds:

// (A) to make moment optional
define('moment', function() {});
require(['chartjs'], function(Chart) {});

// ... or ...

// (B) to correctly load moment, thus enable the time scale
require(['moment'], function() {
    require(['chartjs'], function(Chart) {});
});

I feel that the (A) use cases are more natural and expected than having to explicitly pre-load moment before and in a separate require() block (B).

[benmccann]

Of course that's only in the case where they want to use the time scale and include moment separately themselves. That's a pretty advanced use case, so I think asking for some additional configuration is not that burdensome in that case. If they just want to use the timescale out-of-the box then they can use the bundled version as an easier solution most of the time

We also should give consideration to users of the tools besides Require.js. For example, I believe that Webpack users cannot make moment optional on master currently. Webpack is far more popular (source), so we should give at least as much consideration to it.

One solution would be to create three builds Chart.js (A), Chart.optional.js (B), and Chart.bundle.js and set the main in package.json to Chart.optional.js

[simonbrunel]

Not sure why the comparison between Webpack and Require.js: if I'm not wrong, Webpack uses the CJS path while Require.js uses the AMD path. My two previous comments are about the AMD wrapper only, Webpack is not impacted whatever solution we select.

That's a pretty advanced use case ...

I don't think so, it has been reported multiple times, it allows to use a single (and latest) version of moment for their whole app, so I would consider this use case as common as with Webpack or rollup.

... they can use the bundled version ...

We should stop promoting this Chart.bundle.js version, it causes many confusion. We will also certainly drop it at v3 when removing moment from the core repo.

For example, I believe that Webpack users cannot make moment optional on master currently.

If this PR does things correctly and is merged, they will be able to do that.

@etimberg @kurkle @nagix what do you guys think about (A) and (B) behaviors based on this comment?

I'm still unsure about which one is better.

[benmccann]

I misinterpreted "the rollup built-in wrapper" in the description of Option A to mean that we would abandon this PR as option A, so disregard my last comment.

Thanks for clarifying that we're just talking about what the new rollup plugin should output. Both seem acceptable to me. I haven't used AMD enough to have an opinion on which is more expected, so I'll let the others chime in

[kurkle]

So after another cup of coffee and adjusting my mind to JS world, I deleted the previous comment (it was useless).

And after a talk with SB in slack, I learned require('chart.js', function() is working with 2.7.3 (no time scale) and usage with moment has required workarounds. So that makes me vote for B

[simonbrunel]

@kurkle thanks for the chat :)

I was thinking "what should be the proper way to load chart.js + moment using Require.js" (A), but realized that 2.7.3 already allows to use Chart.min.js without moment, so making it required would break these projects.

Finally (B) seems a better solution until v3 and while iterating on this topic, I found another way to correctly load moment using require.js shim config:

require.config({
    shim: {
        'chartjs': {deps: ['moment']}   // enforce moment to be loaded before chartjs
    },
    paths: {
        'chartjs': 'path/to/chartjs',
        'moment': 'path/to/moment'
    }
});

require(['chartjs'], function(Chart) {
    new Chart( ... );  // time scale works as expected 
});

[benmccann]

@simonbrunel just FYI, this PR will need to be rebased

[lsn793]

sorry for dummy question: How to get build with this fix?
I cloned Master, built it, but still have dist files same size like 2.7.3 version

[simonbrunel]

It doesn't change the size of the built files but allows to use Chart.min.js without moment in a CJS/AMD environment. How do you include Chart.js in your project?

[benmccann]

Please see the latest docs for details: https://github.com/chartjs/Chart.js/blob/master/docs/getting-started/integration.md

[lsn793]

Please see the latest docs for details: https://github.com/chartjs/Chart.js/blob/master/docs/getting-started/integration.md

I use chartjs with angular-cli. So I build angular app like ng build --prod. And anguar cli does not use not webpack nor Rollup. So how can I configure angular project to exclude moment?

[lsn793]

look like it is imposible to use externals and angular cli
angular/angular-cli#8607

[simonbrunel]

@lsn793 I'm not familiar with angular / angular-cli, can you share a/your project that uses chart.js?

[benmccann]

@lsn793 Can you also try removing the package after install and seeing if complication will succeed in that case? See #5978 (comment)

[lsn793]

@benmccann I tried as you advised but got error while build
WARNING in ./node_modules/chart.js/dist/Chart.js
Module not found: Error: Can't resolve 'moment' in 'C:\Users\Serg\dev\calc-weight\node_modules\chart
.js\dist'

And chart does not work.

Browser console output:
ERROR TypeError: chart_js__WEBPACK_IMPORTED_MODULE_1__.Chart is not a constructor

[lsn793]

@simonbrunel https://github.com/lsn793/calc-weight

[jonrimmer]

EDIT: So my original suggested workaround was to use the file replacement capability of angular.json, however, I believe I've found a better solution: Using the browser field of your project's package.json file.

Webpack, including the version wrapped by Angular CLI, processes this field, and it can be used to shim or exclude modules from the browser bundle. I believe it is intended as a way to replace browser-incompatible dependencies when using libraries intended for the server-side, but it works for our purposes as well.

What you need to do is to set moment to false:

  "browser": {
    "moment": false
  }

This will exclude moment.js from the bundle, and make any import or require call to it resolve to an empty object. Unfortunately, right now this causes Chart.js to think Moment is available and throw an error, but this will be fixed by #6113.

[jonrimmer]

FWIW I would also propose making a breaking change that changes Moment to a peer dependency, releasing it as v3.0.0, and making v4 the future "big" release.

I understand the desire to make the version numbers correspond to "important" releases with lots of features, but I'm not sure that desire should override making the correct technical solution. In the end, a version number is just a version number.