-
Notifications
You must be signed in to change notification settings - Fork 323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set the plugin as module #838
Conversation
@kurkle @LeeLenaleee not sure if this is the right way to address all these different imports from different platforms/frameworks. From your experiences, is there a list of "exports" to use for all frameworks? In my opinion it could be also helpfull to have a common approach for all extensions in chartjs org because these issues shuld happen on other plugins/controllers as well, I guess. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
node should use import/require of these automatically. This does not seem like a right way to go.
In the repo, that I use to test, is using import, therefore esm but there is that error specify in the issue
Source import Chart from 'chart.js'
import annotation from 'chartjs-plugin-annotation'
Chart.register(annotation) |
@kurkle I think the best could be to set the plugin as a module. This is also solving the issue but at the same time is creating some issue on other framework (like next). Unfortunately I don't have enough experience on that... sorry. |
I have to admit that I cannot understand why it's not working "type": "module" in next (v13).. I have tried to add treemap 2.3.0 in next app and it works. Then I have changed the plugin package.json file using the same of treemap but it's still not working... I need time to think about. |
Finally I was able to use the plugin (set to type: module) in NextJS, by dynamic import. It's still not clear to me way this is needed but I'm having a look. EDIT: dynamic import is not needed. I forgot only to clear nextjs cache :( |
For frameworks (react, vue, etc.), its essientally the bundler used (webpack/vite/etc.), that makes the difference, and it would probably be less work to make integration tests with bundlers instead of frameworks. |
Yes, agree. I will have a look to what we are doing in Chart.js where I think there some integration test cases but later. Before I want to understand if we need to go to type: module or not and prepare again (#815) a PR to go there :( |
@kurkle @LeeLenaleee I have done some tests and it seems working (I have used some repos I had related to some previous issues). I have used the same "config" of treemap. Just a doubt. I think that with these changes, the dependency with CHART.JS 4 should be mandatory. What do you think? |
Conflicts: docs/.vuepress/config.js
@kurkle about breaking change, do you think we could avoid it if we will create and publish the same files in NPM ( |
Any updates on this issue friends. As of now, I am currently solving by adding "node": "./dist/chartjs-plugin-annotation.min.js" on package.json |
@echo178 this PR is creating a breaking change ( |
Conflicts: docs/.vuepress/config.js
Conflicts: docs/.vuepress/config.js karma.conf.js
@kurkle I have added the test cases for node module and commonjs (slightly different comparing with matrix project). I think we should take a decision if to release this PR in a minor or not. If the breaking change is only the missing |
I have changed rollup config to create |
Just a drive-by comment to say that I've been patching my chartjs-plugin-annotation module locally to use I'm not aware that the |
@kurkle change |
Fix #837