chartjs / chartjs-plugin-deferred

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

Unable to es6 import plugin #2

Closed powerbuoy closed 7 years ago

powerbuoy commented 7 years ago

I'm using ChartJS in my Aurelia app and it works fine. I wanted to add the deferred plugin so I've npm installed it and added the import like so:

import Chart from 'chartjs';
import 'chartjs-plugin-deferred';

The package imports properly but the code throws errors: Uncaught TypeError: Cannot read property 'helpers' of undefined.

My guess is it's because the plugin is trying to access the Chart object which I assume isn't global?

(The first few lines of code try to do the following):

var Chart = window.Chart;
var helpers = Chart.helpers;

I'm honestly a little new to using import (as opposed to just stacking <script> elements) so I might be doing something wrong, but I'd love to know what?

Edit:After further testing the plugin actually seems to work properly, but I still get that error in the console which is really annoying. Any idea what could cause it?

simonbrunel commented 7 years ago

@powerbuoy I always assumed that this plugin would be loaded via a <script> element, after that Chart.js is fully loaded, so I'm not sure that the current implementation is compatible with import. Are you able to share a live test case (or sources) that reproduce this issue?

powerbuoy commented 7 years ago

@simonbrunel Thanks for the quick reply. It's so strange that it actually works, regardless of the errors thrown.

Aurelia has its own bundling process so it may actually have to do with that but I'm not sure.

I tried just running:

import Chart from 'chart.js';
import 'chartjs-plugin-deferred';

console.dir(Chart);

On https://esnextb.in/ and that (unfortunately, for me :P) works error free.

If I reverse the order of the imports I get the same error as I do locally however, so for some reason Aurelia's bundler doesn't work the same as the one on esnextb.in.

In my Aurelia app I don't actually even have to import your plugin to get the error, adding it to my dependencies is enough.

Unless you're an Aurelia pro I guess you know about as much as I do about the problem though.

simonbrunel commented 7 years ago

I don't know Aurelia, so can't really help on that and I'm not sure why the plugin still work even with the thrown error (since it still needs to access Chart.helpers)! Let me know If you figure out the reason of this error, I'm sure we can adapt this plugin to load without any issue.

powerbuoy commented 7 years ago

Yes it's puzzling. I've now tried with a few other ChartJS plugins (annotations, zoom and draggable) and none work. In fact when trying to include them the entire build fails, whereas with yours at least the code is built and runs as expected it's just that I get that error in the console.

There are so many module loaders and ways to do these things nowadays that I'm not surprised things don't always work as expected, but I find it a little hard to believe I'm the first person trying to import a ChartJS plugin using an ES6 import.

zewa666 commented 7 years ago

The issue is pretty simple to be solved. The library should expose a UMD compatible format, that way the consumer can define when the execution has to happen.

By having the build artifact wrapped inside UMD, we can as well declare dependencies, respectively ChartJS, and as such leverage the ES6 import syntax as well.

Over at the Aurelia CLI your aurelia.json deps would then look like:

           {
            "name": "chart.js",
            "path": "../node_modules/chart.js/dist",
            "main": "Chart.min",
            "exports": "Chart"
          },
          {
            "name": "chartjs-plugin-deferred",
            "path": "../node_modules/chartjs-plugin-deferred/dist",
            "main": "chartjs-plugin-deferred.umd",
            "deps": ["chart.js"]
          },
simonbrunel commented 7 years ago

Thanks @zewa666, it may be interesting to also comment on chartjs/Chart.js#4160 since it can be useful for plugin maintainers?

simonbrunel commented 7 years ago

@powerbuoy can you pull @zewa666 branch from #3 and confirm that it fixes your issue with Aurelia?

zewa666 commented 7 years ago

@simonbrunel yep could be ... as you said lets @powerbuoy first test the example and then we can see how to generalize this for other plugin maintainers. For your example it was pretty easy since you had a nice infrastructure with Gulp in place, for others it might be more work to propose how to properly add UMD support by themselves.

powerbuoy commented 7 years ago

@simonbrunel @zewa666 Thanks for your efforts guys! I might be doing something wrong, but here are the steps I've taken:

1) Updated my package.json to point to the PR; "chartjs-plugin-deferred": "github:chartjs/chartjs-plugin-deferred#pull/3/head" 2) Re-ran npm install and made sure I had the latest code 3) Updated my aurelia.json to the suggestion in the comment above, however that didn't work because there's no chartjs-plugin-deferred.umd.js file in there 4) Tried with my previous aurelia.json:

    {
        "name": "chartjs",
        "path": "../node_modules/chart.js/dist/Chart.min"
    },
    "chartjs-plugin-deferred",

5) With that I still get the exact same error.

I've noticed that with Aurelia CLI's build system, a package mustn't contain a "." or it fails to import, that's why I keep renaming ChartJS like that.

I'd love to understand why I still need the exports and deps properties though, because none of the other packages I use require anything like it?

It also seems the PR is missing the dist/ folder, should I run gulp myself or?

simonbrunel commented 7 years ago

Yes, to test the PR, you need to run gulp build and use the generated dist/chartjs-plugin-deferred.js. Once released, this file will be automatically built and pushed to the tag and npm package.

zewa666 commented 7 years ago

@powerbuoy also please disregard chartjs-plugin-deferred.umd.js that was just me copying the newly resulting file over to my existing repo. Packages including dots shouldn't be a problem

powerbuoy commented 7 years ago

I see. Ok I've npm installed and gulped and now have a dist/ folder.

I've also changed my bundle file to point to the new dist/ folder:

{
    "name": "chartjs-plugin-deferred",
    "path": "../node_modules/chartjs-plugin-deferred/dist/chartjs-plugin-deferred",
    "deps": ["chartjs"]
},

(Note that the path shouldn't contain .js in Aurelia CLI).

With this in place the Aurelia build now fails, like other ChartJS plugins I've tried it now looks for the plugin's dependencies inside my project root instead of in node_modules.

Note that this happens with and without the exports and deps properties which tbh I still don't really understand why I need?

zewa666 commented 7 years ago

yep the path should not contain the file itself but only the folder. Main defines the filename you use. Deps and export are used to configure non-es6 packages. Exports defines the global variable which is set by a package, that is Chart from chart.js. Deps on the other hand says what the required package is that has to be finished loading before a package is loaded.

If you still experience issues, can you create a minimal example replicating the issue and upload it to a github library, so I can take a look at it and help you out perhaps.

powerbuoy commented 7 years ago

Tbh it sounds like you guys know plenty more about all these module loaders so I wouldn't be surprised if it's my lack of experience with them that's making me fail. But like I mentioned before, I use plenty of other node modules and they all work without issue (just add the package name to aurelia.json and then import away).

Setting up a brand new Aurelia project is actually quite simple if you would like to test this locally:

1) npm install -g aurelia-cli 2) au new 3) Use the defaults 4) cd project-folder/ 5) au run --watch 6) Site should now be available on localhost:9000 7) Install ChartJS and the Deferred plugin + add them both to aurelia_project/aurelia.json 8) Try au run --watch again and you should be getting the same issues I'm having

powerbuoy commented 7 years ago

Seems we commented pretty much at the same time :) I've tried with this as well:

{
    "name": "chartjs",
    "path": "../node_modules/chart.js/dist",
    "main": "Chart.min",
    "exports": "Chart"
},
{
    "name": "chartjs-plugin-deferred",
    "path": "../node_modules/chartjs-plugin-deferred/dist",
    "main": "chartjs-plugin-deferred",
    "deps": ["chartjs"]
},

And I still get the same error about chart.js not being available inside my project root; Error: ENOENT: no such file or directory, open '/Users/powerbuoy/project-folder/chart.js'

Note that including ChartJS itself has never been an issue though.

zewa666 commented 7 years ago

Please try to change the chartjs name to "chart.js" and also use the same name in the deps property.

powerbuoy commented 7 years ago

Unfortunately that doesn't work with Aurelia CLI; https://github.com/aurelia/cli/issues/349

simonbrunel commented 7 years ago

UMD is setup with dependencies over the chart.js package. It seems correct but I think that's why your "name": "chartjs" alias is ignored while loading the plugin module, which still try to load chart.js.

I don't know if the "dot in name" is a limitation of Aurelia (and so should be fixed on their side) or, more generally, an issue with module loaders (e.g. can't find any restriction in the node guide, except that a name must not begin with a dot).

powerbuoy commented 7 years ago

I see. It wouldn't surprise me if this is an Aurelia issue tbh. Renaming packages like I do above has worked for me in the past though (with jump.js). But I guess if I install another package that depends on the renamed package this might happen?

powerbuoy commented 7 years ago

Ok some progress :) If I change:

dependencies: function(file) {
  return [{
    name: 'chart.js',
    amd: 'chart.js',
    cjs: 'chart.js',
    global: 'Chart',
    param: 'Chart'
  }];
}

To:

dependencies: function(file) {
  return [{
    name: 'chartjs',
    amd: 'chartjs',
    cjs: 'chartjs',
    global: 'Chart',
    param: 'Chart'
  }];
}

It works :)

So I guess I'll need to reopen https://github.com/aurelia/cli/issues/349 and take it from there. And I guess this PR works perfectly for build systems that don't have the "." bug.

Thanks a lot for your help guys, let's hope the Aurelia team is as helpful :D

zewa666 commented 7 years ago

So I'm still not sure why you experience the dot issue, but as you said that is a Aurelia CLI specific issue and not part of this. The library is named chart.js as such it should also be exported like that. And since this package does refer to the original name, renaming it in the CLI won't help. So I wouldn't change this PR here but rather fix it over on our side in the CLI project.

Regarding that @powerbuoy I'd be really glad if you could create a minimal example to reproduce the issue and upload it somewhere. I tried setting up an example with the Aurelia CLI including the dot in the name and everything worked. So having your example would clarify whether I did something wrong or its really an issue.

powerbuoy commented 7 years ago

Alright I've set up a brand new Aurelia CLI project and done nothing but add chart.js to it.

If you add "chart.js" to the dependencies array inside aurelia.json you should see the same issue I am when trying to run au run. If you change "chart.js" to "chartjs" (and supply a path) the build works.

chartjs-aurelia-example.zip

Edit: Btw, I never meant the PR should change to "chartjs". I realise this is no longer ChartJS's problem but rather a problem with Aurelia's build system. I'm surprised it works for you though, I'm running the latest version of au-cli (0.28.0)

zewa666 commented 7 years ago

Your aurelia.json should have this at the end of the dependencies section:

{
  "name": "chart.js",
  "path": "../node_modules/chart.js/dist",
  "main": "Chart.min"
}

and the package needs to be installed with npm install --save chart.js

my package.json deps section then looks like follows:

"dependencies": {
    "aurelia-animator-css": "^1.0.1",
    "aurelia-bootstrapper": "^2.1.0",
    "bluebird": "^3.4.1",
    "chart.js": "^2.5.0",
    "requirejs": "^2.3.2",
    "text": "github:requirejs/text#latest"
  },

and everything works as expected for me. Using v 0.28.0 as well

chartjs-aurelia-example-updated.zip

powerbuoy commented 7 years ago

Oh man I never even considered I could keep using the "chart.js" name so long as I supplied the path manually. Especially since the creator of Aurelia told me not to use a period in the name in the referenced Aurelia CLI issue.

All works perfect now! Thanks so much for your help guys :)

zewa666 commented 7 years ago

hehe I guess that was a bit of a misunderstanding from Rob. The path should not end with .js because that most likely indicates you've tried to put the file along the path into the path info. So graph.js is a crazy example since the folder really is named graph.js, and without a provided main entry point Aurelia CLI thinks you meant that the default main index.js should be taken.

Alright, glad we've been able to clarify that, and sorry @simonbrunel for abusing your repos issue for that :)

Give me a ping once you've decided how to include the PR and we can chime in on the referred issue to aid other plugin maintainers with an example

@powerbuoy do not forget to update the stackoverflow issue btw :)

simonbrunel commented 7 years ago

Fixed in d22f5a0a6b855, @powerbuoy can you confirm that the plugin now correctly loads in Aurelia when using master? You still need to gulp build in the plugin directory, until a new version is released. I also changed the package.json:main value to target the generated UMD file, so you should be able to simply add "chartjs-plugin-deferred" to your aurelia.json dependencies:

dependencies: [
  {
    "name": "chart.js",
    "path": "../node_modules/chart.js/dist",
    "main": "Chart.min"
  },
   "chartjs-plugin-deferred"
]
powerbuoy commented 7 years ago

Can confirm it works, and I'm using exactly the same aurelia.json as in your comment above (I no longer seem to need chart.js's exports property and chartjs-plugin-deferred can be included like all my other packages without specifying a path or deps).

Awesome work 👍

simonbrunel commented 7 years ago

Released in v0.3.0