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

Enhancement: V3 compatibility #21

Closed LeeLenaleee closed 2 years ago

LeeLenaleee commented 2 years ago

Make the plugin compatable with chart.js V3

LeeLenaleee commented 2 years ago

@simonbrunel If you have time can you check this PR out?

simonbrunel commented 2 years ago

@LeeLenaleee it looks like the package-lock.json contains update for all dependencies instead of only chart.js. I'm not sure how you generated it, but I think you just need to delete it and run npm install (and not npm update).

LeeLenaleee commented 2 years ago

I first ran npm install -g npm@6.14.15 to downgrade npm, after that I removed the package-lock.json and ran npm install and this was the result, will try some other things.

nader-fl commented 2 years ago

Thanks for all the effort put into this, looking forward to the update

LeeLenaleee commented 2 years ago

@simonbrunel I can't get the lockfile to not change the integratie check from sha1 to sha256, only way I can think of to achieve this is by after running npm install copy the old package-lock.json and paste it in the root, change the part about chart.js manually and commit it.

But I don't think that is a good way of fixing this before you do a deps update.

So do you want to have it like this or only the chart.js change?

simonbrunel commented 2 years ago

@LeeLenaleee I didn't get notified about your last commit so I missed your update. I will try to fix the package-lock on merge before the end of the week/year.

LeeLenaleee commented 2 years ago

Yo np, take your time, also happy holidays still :)

gregh3269 commented 2 years ago

Good to get this on V3, so thanks. ...But

Where does chart.js/helpers come from? If I build from LeeLenaleee:feature/v3-compatible and test it I get: Uncaught TypeError: helpers is undefined. on var overflowX = helpers.getStyle(node, 'overflow-x');

Cheers

LeeLenaleee commented 2 years ago

@gregh3269 chart.js V3 is treeshakable and the helpers are exported from a sub part for ESM builds instead of from the main chart.js namespace.

So if you look at this line: https://github.com/chartjs/chartjs-plugin-deferred/blob/13ca7f8b86b6e1b834decc0f8b0de08ed21c416f/src/plugin.js#L4

You see I use treeshaking to import only the 2 helper functions I need to use them later.

After importing them this way I never define helpers so that's why you get the error. Instead I can just call these functions like they where local functions as you can see here: https://github.com/chartjs/chartjs-plugin-deferred/blob/13ca7f8b86b6e1b834decc0f8b0de08ed21c416f/src/plugin.js#L80

But since you get those errors it almost seems like you didn't pull my code changes correctly since it still has the V2 compatible syntax in there instead of my changes

gregh3269 commented 2 years ago

@LeeLenaleee I downloaded the branch as a zip then

npm install npm run build

chartjs-plugin-deferred@1.0.2 build /./chartjs-plugin-deferred-feature-v3-compatible rollup -c src/plugin.js → dist/chartjs-plugin-deferred.js, dist/chartjs-plugin-deferred.min.js... (!) Missing global variable names Use output.globals to specify browser global variable names corresponding to external modules chart.js/helpers (guessing 'helpers') chart.js/helpers (guessing 'helpers') (!) Unresolved dependencies https://rollupjs.org/guide/en/#warning-treating-module-as-external-dependency chart.js/helpers (imported by src/plugin.js) created dist/chartjs-plugin-deferred.js, dist/chartjs-plugin-deferred.min.js in 450ms

Is there anything else on the chart.js I need to do?

LeeLenaleee commented 2 years ago

@gregh3269 Never actually looked at the build log because I started it with npm run docs:dev so totally missed that warning because that clears the console.

Added it to rollup so warning is gone.

gregh3269 commented 2 years ago

@LeeLenaleee not sure how you are testing this,

but from the latest build, changing the lines below:

(global = typeof globalThis !== 'undefined' ? globalThis : global || self, factory(global.Chart, global['Chartjs Helpers'])); to (global = typeof globalThis !== 'undefined' ? globalThis : global || self, factory(global.Chart, global.Chart.helpers));

makes it work correctly for me from the zip/build

Cheers Greg

LeeLenaleee commented 2 years ago

Well I don't know what goes wrong for you, got no warnings when running npm run build also even when I got the warnings it worked fine, after removing the warnings it works fine.

gregh3269 commented 2 years ago

@LeeLenaleee latest build now works :-)

https://github.com/LeeLenaleee/chartjs-plugin-deferred/commit/5e7fb88bf945027b30e9dbc3dbd523805ecb5595

Good work.

Cheers Greg

LeeLenaleee commented 2 years ago

You should have access already, box allow edits my maintainers was already checked

simonbrunel commented 2 years ago

Thank you @LeeLenaleee for this PR, I will try to release a beta version soon.

And thanks @gregh3269 for your tests.

LeeLenaleee commented 2 years ago

@simonbrunel do you have an idea on when you are able to release a beta?

simonbrunel commented 2 years ago

@LeeLenaleee In order to release a beta, I would like to remove the auto-registration of the plugin and export it instead (as explained in this comment). Unfortunately, I currently have no time to allocate to this project so if someone wants to submit a PR for this change, it would be really appreciated.

simonbrunel commented 2 years ago

@LeeLenaleee Version 2.0.0-beta.1 has been released.

Thank you again for your help!