-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
base: master
Are you sure you want to change the base?
Conversation
rollup.config.js
Outdated
}, | ||
|
||
// UMD build | ||
// dist/chart.umd.js (old filename, deprecated) |
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.
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.
Yeah, I wouldn't deprecate this now.
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.
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`. |
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.
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)
.
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.
If they're upgrading to an older version won't they be seeing that older version's migration guide?
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.
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
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.
There are arguments for both approaches. And I support either.
Here is a PR for the other wording: #11470
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.
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
cfb968f
to
77c65d0
Compare
This names our already minified UMD dist file to the format that jsDelivr specifies for this type of file.