Skip to content
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

Compile for CDN (and SRI) #11455

Open
fulldecent opened this issue Aug 18, 2023 · 5 comments · May be fixed by #11457
Open

Compile for CDN (and SRI) #11455

fulldecent opened this issue Aug 18, 2023 · 5 comments · May be fixed by #11457

Comments

@fulldecent
Copy link
Contributor

Feature Proposal

This is the last version of Chart.js which supports CDN with SRI:

https://cdn.jsdelivr.net/npm/chart.js@3.9.1/dist/chart.min.js

Currently, there is no upgrade path for this version.

Can we please again release minified CDN versions which can be safely delivered with SRI?

Possible Implementation

No response

@fulldecent
Copy link
Contributor Author

Also can I please get permissions back on this repo so I can triage issues again?

@LeeLenaleee
Copy link
Collaborator

I don't have any knowledge of SRI, so maby I am seeing this wrong but https://cdn.jsdelivr.net/npm/chart.js@4.3.3/dist/chart.umd.js is already minified so if the minification is the issue this is the solution right?

@fulldecent
Copy link
Contributor Author

Cool, thank you. I just tried running that file and it is working with SRI. The issue here might be we are not using the expected convention.

For example, on all versions up to 3.9.1 our main UMD file is detected. Example:

https://www.jsdelivr.com/package/npm/chart.js?tab=files&version=3.9.1

Screenshot 2023-08-18 at 12 56 39

However, on more recent versions, our UMD file is not found. jsDelivr is taking it upon itself to "minify" this file for us because it cannot find the minified file.

https://www.jsdelivr.com/package/npm/chart.js?tab=files&version=4.3.3

Screenshot 2023-08-18 at 12 56 53


Maybe the solution is we need to name the file differently or specify in package.json. I'm investigating now.

@fulldecent
Copy link
Contributor Author

I see that we have specified it here:

https://github.com/chartjs/Chart.js/blob/v4.3.3/package.json#L13-L15

And in the past we did similarly:

https://github.com/chartjs/Chart.js/blob/v3.9.1/package.json#L7-L9


jsDelivr documentation on this is at: https://www.jsdelivr.com/documentation#id-configuring-a-default-file-in-packagejson

So the problem is that jsDelivr is expecting a minified JavaScript file will always have the extension *.min.js. Whereas we are publishing chart.umd.js.

I think the solution here is to follow established conventions and that we should publish this file as chart.umd.min.js.

This could even be a copy of the existing file if that makes publishing easier.

fulldecent added a commit to fulldecent/Chart.js that referenced this issue Aug 18, 2023
@LeeLenaleee LeeLenaleee linked a pull request Aug 18, 2023 that will close this issue
fulldecent added a commit to fulldecent/Chart.js that referenced this issue Sep 6, 2023
fulldecent added a commit to fulldecent/Chart.js that referenced this issue Sep 6, 2023
@lonix1
Copy link

lonix1 commented Feb 15, 2024

Agreed, it was a real pain to set up when using a CDN.

The minified file chart.umd.js does not play well with CDN and other tools. It should be chart.umd.min.js or chart.min.js.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants