Skip to content

Update packaging to match main chart.js library #815

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

Closed
wants to merge 8 commits into from

Conversation

stockiNail
Copy link
Collaborator

Fix #814

@stockiNail stockiNail added this to the 2.1.1 milestone Dec 5, 2022
Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok, but donwe really want to rename the umd files?

@stockiNail stockiNail marked this pull request as draft December 9, 2022 13:03
@stockiNail stockiNail marked this pull request as ready for review December 9, 2022 15:05
@stockiNail
Copy link
Collaborator Author

@kurkle I wen back to the original version, without using UMD in the dist files, as shared. Let me know if it's ok and if yes, let me know if you think we should publish new version 2.1.1 (bumping).

kurkle
kurkle previously approved these changes Dec 14, 2022
Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, except it won't work in node/commonjs.

@etimberg is it ok for you to drop cjs support for plugins too? (main repo already did)

package.json Outdated
"main": "dist/chartjs-plugin-annotation.js",
"module": "dist/chartjs-plugin-annotation.esm.js",
"type": "module",
"main": "dist/chartjs-plugin-annotation.esm.js",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to support chart.js v3 on node/commonjs with these later annotation plugin versions, this needs to be umd build with .cjs extension

Something like I did in autocolors: https://github.com/kurkle/chartjs-plugin-autocolors/blob/main/package.json

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok Let me update in next hours

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kurkle updated as suggested

@etimberg
Copy link
Member

Looks good to me, except it won't work in node/commonjs.

@etimberg is it ok for you to drop cjs support for plugins too? (main repo already did)

Dropping is fine by me, but should definitely be a major version release

@stockiNail
Copy link
Collaborator Author

Looks good to me, except it won't work in node/commonjs.
@etimberg is it ok for you to drop cjs support for plugins too? (main repo already did)

Dropping is fine by me, but should definitely be a major version release

@etimberg ok that means all plugins/controllers in chartjs org must publish a major version for chartjs 4 compatibility, doesn’t it?

@etimberg
Copy link
Member

Looks good to me, except it won't work in node/commonjs.
@etimberg is it ok for you to drop cjs support for plugins too? (main repo already did)

Dropping is fine by me, but should definitely be a major version release

@etimberg ok that means all plugins/controllers in chartjs org must publish a major version for chartjs 4 compatibility, doesn’t it?

I don't think it's mandatory if the plugin packaging doesn't change

@stockiNail
Copy link
Collaborator Author

@etimberg @kurkle with CHART.JS 4.1.1 this PR sounds to me useless because the issue should be fixed.

@etimberg etimberg closed this Dec 19, 2022
@kurkle
Copy link
Member

kurkle commented Dec 19, 2022

There could be some benefits to from type: 'module', but maybe nothing worth the time before someone requests something explicit.

@stockiNail stockiNail removed this from the 2.1.1 milestone Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Server Error when using annotation plugin with Chart.js 4.0.1 in NextJS app
4 participants