Set the plugin as module

[stockiNail]

Fix #837

[stockiNail]

@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.

[stockiNail]

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

node index.js
(node:9264) Warning: To load an ES module, set "type": "module" in the package.json or use the .mjs extension.
(Use `node --trace-warnings ...` to show where the warning was created)
C:\Users\AndreaStocchero\git\chartjs-annotation-bug-test\node_modules\chartjs-plugin-annotation\dist\chartjs-plugin-annotation.esm.js:7
import { Element, defaults, Animations, Chart } from 'chart.js';
^^^^^^

Source

import Chart from 'chart.js'
import annotation from 'chartjs-plugin-annotation'

Chart.register(annotation)

[stockiNail]

@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.

[stockiNail]

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.

[stockiNail]

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. That said, I think the plugin should be marked as module, which solves the issue https://github.com/chartjs/chartjs-plugin-annotation/issues/837 Furthermore I thin we should need to extend the integration test cases with some other frameworks (like react, next, vue) in order to be sure that it's working on those platforms. Make sense?

EDIT: dynamic import is not needed. I forgot only to clear nextjs cache :(

[kurkle]

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.

[stockiNail]

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 (https://github.com/chartjs/chartjs-plugin-annotation/pull/815) a PR to go there :(

[stockiNail]

@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?

[stockiNail]

@kurkle about breaking change, do you think we could avoid it if we will create and publish the same files in NPM (chartjs-plugin-annotation.js is now missing)?

[echo178]

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

[stockiNail]

@echo178 this PR is creating a breaking change (chartjs-plugin.annotation.js --> chartjs-plugin.annotation.cjs), therefore it has been planned for next major version. @kurkle @LeeLenaleee thoughts?

[stockiNail]

@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 chartjs-plugin.annotation.js dist file, I think we could create it as well (compatibility with the past). About minimum Chart.js dependencies, I think we had already done it with version1 where we changed that in a minor. After the decision, I think we should bump to version 2.2.0 (with or without this PR). What do you think?

[stockiNail]

The some of the entry points are renamed so its a breaking change. I would not wait with it too long though, so maybe soon after 2.2 go for 3?

Or could just move all the 2.2 stuff to 3 instead and publish all at once.

I have changed rollup config to create chartjs-plugin-annotation.js as well. From user perspective, I don't see breaking changes related to the renamed file now. I would like to go to next version anyway, including it. @kurkle just a hint: do you think it's better to go to version 3 or publish 2.2 with this one (remove breaking change, if there isn't)? For me no problem, just a matter to change the bump PR and some milestones, already assigned, before publishing.

[donmccurdy]

Just a drive-by comment to say that I've been patching my chartjs-plugin-annotation module locally to use "type": "module", and that fixed build errors in my build (SvelteKit + Vite) here. Looking forward to using the next release, and let me know if any help is needed.

I'm not aware that the "type": "module" and .js → .cjs updates would be a breaking change for package consumers using npm and bundlers. For consumers using CDNs it may be a breaking change, though.

[stockiNail]

@kurkle change peerDependencies to Chart.js. after some tests, it works only with Chart.js 4.x.x. Furthermore, added migration guide to v3 and changed the mentioning to Chart.js version dependency in README and doc index. When you have time, have a look (also the other PR for v3 even if the migration guide, added here, should be changed by the other PR).