search

reactjs / react-chartjs

common react charting components using chart.js
MIT License
2.93k stars 299 forks source link

0.2.1: Using Webpack throws: The charts were not initialized with the React instance #4

Closed seanadkinson closed 9 years ago

seanadkinson commented 9 years ago

It says in issue #3 that you wouldn't need to do the manual settings of the vars thing in 0.2.0, but I have 0.2.1 installed via NPM, and still getting this issue on a very simple example.

Webpack doesn't have the require function at runtime, so the typeof require statements are failing in the code that should dynamically require react (https://github.com/jhudson8/react-chartjs/blob/master/vars.js#L59).

Seems like there is an opportunity to make this a lot easier for NPM users, since react and chart.js could just be specified as peerDependencies. I may have a PR for you at some point, but perhaps you already have a better idea how this could work better.

I noticed that react-chartjs.js is actually compiled with webpack, but I'm not sure how to provide it global.Chart and global.React. Any hints would be appreciated. Thanks.

seanadkinson commented 9 years ago

For what I wanted/expected this library to do, it just wasn't quite there. I've changed things around a bit in the fork I just made, and published to npm as react-chartjs-commonjs, since it really only supports CommonJS well.

Repo here: https://github.com/seanadkinson/react-chartjs

I may break it out to it's own repo with another name (giving original credit to your still of course), unless you think you want/can incorporate the changes I made.

Basically I made chart.js and react peer dependencies, and included them using require in the createClass.js file (which was renamed from vars.js). This reduced a lot of unecessary checks on global. Then I just removed all the non-commonjs stuff, and moved most of the files into a lib directory. The last change was just exporting each module from the index.js, but this isn't really needed that badly.

Let me know what you think, or if you want to work together on making these changes part of this repo, and then I'll delete mine.

Thanks for the work you did on this.

jhudson8 commented 9 years ago

sure, feel free to PR it - I welcome additions

seanadkinson commented 9 years ago

My changes would make it so that it only really supports CommonJS, though, so I'm not sure if you want them merged in. I'll open a PR and you can look at it, but I deleted more than I probably should have, thinking it would end up being a separate, so I can put some things back if you need em.

andrinealver commented 9 years ago

:+1: Thanks @seanadkinson - saved our day :)

KaeruCT commented 9 years ago

:+1: another thanks from me @seanadkinson

jhudson8 commented 9 years ago

ok, I may take a look at it this weekend.

seanadkinson commented 9 years ago

Yeah, sorry I haven't circled back on this @jhudson8. I was using this on a side project, not my day job, so I just haven't carved out time to dig into supporting both CommonJS and AMD modules.

jhudson8 commented 9 years ago

I've published a 0.3.0 release which should work well for commonjs/webpack/browserify. Please let me know if there are any issues.

@seanadkinson obviously you can do what you want but, unless there is some reason to have a separate published fork, I'd appreciate if you direct people to this project just to fragment things. If you'd like to be a maintainer on this project, that's fine.

jhudson8 commented 9 years ago

no worries. I only use commonjs (and had to put my side project on hold which is why I didn't notice the issue). I agree with you in the fact that people are really (or at least should be) going to be using commonjs.

seanadkinson commented 9 years ago

Cool, I'll upgrade to 0.3.0 this weekend, and assuming it all works okay, I'll delete my fork and npm module.

@andrinealver and @KaeruCT, please upgrade to 0.3.0 of this repo, as I won't be maintaining react-chartjs-commonjs if this repo does the same thing.

Thanks @jhudson8!

jhudson8 commented 9 years ago

No problem. I tested it out and it should be good but let me know of you have any problems

On Fri, Feb 6, 2015, 6:22 PM seanadkinson notifications@github.com wrote:

Cool, I'll upgrade to 0.3.0 this weekend, and assuming it all works okay, I'll delete my fork and npm module.

@andrinealver https://github.com/andrinealver and @KaeruCT https://github.com/KaeruCT, please upgrade to 0.3.0 of this repo, as I won't be maintaining react-chartjs-commonjs if this repo does the same thing.

Thanks @jhudson8 https://github.com/jhudson8!

— Reply to this email directly or view it on GitHub https://github.com/jhudson8/react-chartjs/issues/4#issuecomment-73331687 .

pghalliday commented 9 years ago

I notice that chart.js still needs to be added globally. This seems unnecessary if using commonjs. I worked around it by npm installing chart.js and adding the following to a module as chart.js does support commonjs

window.Chart = require('chart.js');

Seems that it would be better to add chart.js to peerDependencies and require it in core similar to @seanadkinson 's approach

jhudson8 commented 9 years ago

I'll do that. thanks @pghalliday - this is stupid but I didn't realize that there was a npm package for chart.js. You should expect a new release tomorrow.