iBet uBet web content aggregator. Adding the entire web to your favor.
iBet uBet web content aggregator. Adding the entire web to your favor.



Link to original content: http://github.com/chartjs/Chart.js/pull/11457
Name artifact chart.umd.js, fixes #11455 by fulldecent · Pull Request #11457 · chartjs/Chart.js · GitHub
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

Name artifact chart.umd.js, fixes #11455 #11457

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fulldecent
Copy link
Contributor

This names our already minified UMD dist file to the format that jsDelivr specifies for this type of file.

rollup.config.js Outdated Show resolved Hide resolved
@LeeLenaleee LeeLenaleee linked an issue Aug 18, 2023 that may be closed by this pull request
rollup.config.js Outdated
},

// UMD build
// dist/chart.umd.js (old filename, deprecated)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wouldn't say it's deprecated. Then we will have a file rename again in V5 and people weren't happy about this one either so I would just let them co exist and not mark any of them deprecated.

@etimberg @kurkle what are your thoughts about this?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I wouldn't deprecate this now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Undeprecated. Not a big deal, it can always be deprecated later.

@@ -30,7 +30,7 @@ A number of changes were made to the configuration options passed to the `Chart`
* Time and timeseries scales use `ticks.stepSize` instead of `time.stepSize`, which has been removed.
* `maxTickslimit` wont be used for the ticks in `autoSkip` if the determined max ticks is less then the `maxTicksLimit`.
* `dist/chart.js` has been removed.
* `dist/chart.min.js` has been renamed to `dist/chart.umd.js`.
* `dist/chart.min.js` has been renamed to `dist/chart.umd.min.js`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure we should edit the migration guide.

Since if you look at the latest docs but use version 4.3 this won't be correct.

We can add a (`dist/chart.umd.min.js` after version 4.5.0).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If they're upgrading to an older version won't they be seeing that older version's migration guide?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, if you click the right link.

But even if you go to 4.5.0 this change would still be technically incorrect since we did not rename it from old to that we renamed it to what's already there and this gets added

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are arguments for both approaches. And I support either.

Here is a PR for the other wording: #11470

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be more in favour of that one, but it wont be a dealbreaker for me.

Seems the test in both pr's fail on that it can't load 'src/chart.umd.min.js':
Uncaught NetworkError: Failed to execute 'importScripts' on 'WorkerGlobalScope': The script at 'src/chart.umd.min.js' failed to load. thrown

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.

Compile for CDN (and SRI)
3 participants