chartjs / chartjs-plugin-deferred

Chart.js plugin to defer initial chart updates
https://chartjs-plugin-deferred.netlify.app/
MIT License
109 stars 21 forks source link

UMD support #3

Closed zewa666 closed 7 years ago

zewa666 commented 7 years ago

Adds UMD support for the libraries build artifacts

Related issue: https://github.com/chartjs/chartjs-plugin-deferred/issues/2

zewa666 commented 7 years ago

As explained in the referenced issue I wouldn't see any easy way, for any module bundler to take full control, over when dependencies are properly timed. So an UMD approach seems to be the best to satisfy a broad variety of module formats.

Please do make sure this change doesn't break any pre-existing use cases, I didn't really find anything special but it would make sense to take some existing examples and properly check those.

I've also seen that you leverage gulptasks, so I felt free to modify those, but you might wanna think about creating a 3rd build articaft containing the umd exports

zewa666 commented 7 years ago

Alright I've added the requested changes. One tip. How about rewriting the gulpfile to ES6 so it could be removed from the eslintignore list?

simonbrunel commented 7 years ago

Looks good, thank, will try to test that very soon! The gulpfile is in the eslintignore because it doesn't use the same indentation rules as the plugin. Though, I already have some changes locally and can have a look to migrate to ES6.

That could be interesting to look at the other Chart.js hosted plugins to make them fully compatible with other environments such as Aurelia ;)

zewa666 commented 7 years ago

@simonbrunel wonder why my local eslint didn't capture that ... strange. Good catch thanks

simonbrunel commented 7 years ago

gulp-umd uses the filename (capitalized) as the global namespace and so tries to set window.Plugin = Plugin, which doesn't look nice. Also, the plugin doesn't export anything and module.exports = undefined doesn't look good either.

It's possible to configure the target namespace but since gulp-umd doesn't do a lot and seems not anymore maintained, I think it's easier to directly implement UMD inside plugin.js:

'use strict';

;(function(root, factory) {
    if (typeof define === 'function' && define.amd) {
        define(['chart.js'], factory);
    } else if (typeof exports === 'object') {
        module.exports = factory(require('chart.js'));
    } else {
        factory(root.Chart);
    }
}(this, function(Chart) {
    var helpers = Chart.helpers;

        // ...

    var plugin = {
        beforeInit: function(instance) {
        // ...
    };

    Chart.plugins.register(plugin);
    return plugin;
}));

@zewa666 what do you think?

zewa666 commented 7 years ago

Fewer dependencies is always better. I was just unsure whether you'd like to have it as part of your gulp build, but if you're fine with placing it in plugin.js it's definitely fine as well

simonbrunel commented 7 years ago

@zewa666 I'm fine with having it in plugin.js, do you want to update that PR or should I implement it in my upcoming changes?

zewa666 commented 7 years ago

hey @simonbrunel, yeah if you don't mind that would be easier. I'll close this PR thanks for the reviews. 👍

simonbrunel commented 7 years ago

Thanks for your help and time :)

simonbrunel commented 7 years ago

After investigating a bit more, I think I will use rollup which generates a better UMD code than gulp-umd.

zewa666 commented 7 years ago

Sure, which also has a lot of traction these days. Good choice