Skip to content

Remove chartjs in favor of chart.js#22442

Merged
1 commit merged intomasterfrom
chartjs
Jan 3, 2018
Merged

Remove chartjs in favor of chart.js#22442
1 commit merged intomasterfrom
chartjs

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Dec 21, 2017 •
edited by ghost
Loading

Fixes #12134
chartjs provides types for the same packages as chart.js, which on NPM is named chart.js. CC @FanaHOVA @JeroenDragt
Looking at the commit history chart.js is much more popular than chartjs, so I'll just assume that its types are more accurate.
If this is merged, I will run npm deprecate @types/chartjs "'@types/chartjs' is now '@types/chart.js'".

@FanaHOVA
Copy link
Copy Markdown
Contributor

FanaHOVA commented Dec 22, 2017

@Andy-MS looks like the other definitions are more updated, but also not sure if they will work as a replacement for the ones we are removing. Any way to test that? Maybe run the current test suite against the other definitions?

@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Dec 27, 2017

After 5 days, no one has reviewed the PR 😞. A maintainer will be reviewing the PR in the next few days and will either merge it or request revisions. Thank you for your patience!

@typescript-bot typescript-bot added the Unmerged The author did not merge the PR when it was ready. label Dec 27, 2017
paulvanbrenk
paulvanbrenk suggested changes Dec 29, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@paulvanbrenk paulvanbrenk left a comment

Choose a reason for hiding this comment

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

See comment by @FanaHOVA

@typescript-bot typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Dec 29, 2017
@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Dec 29, 2017

@Andy-MS One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you!

@typescript-bot typescript-bot removed the Unmerged The author did not merge the PR when it was ready. label Dec 29, 2017
@ghost
Copy link
Copy Markdown
Author

ghost commented Jan 2, 2018 •
edited by ghost
Loading

@FanaHOVA It looks like chartjs may be for an earlier version of the library? (The version number is missing in the header.) The newer one doesn't seem to have a .Line, .Bar., .Radar, etc. methods on Chart. The latest docs use type: 'line' instead.

@mhegazy
Copy link
Copy Markdown
Contributor

mhegazy commented Jan 3, 2018

@Andy-MS please merge and run the depreciation script.

@ghost ghost merged commit ab2b520 into master Jan 3, 2018
@ghost ghost deleted the chartjs branch January 3, 2018 19:54
@ghost
Copy link
Copy Markdown
Author

ghost commented Jan 3, 2018

Ran deprecation script.

@FanaHOVA
Copy link
Copy Markdown
Contributor

FanaHOVA commented Jan 6, 2018

@Andy-MS yup, so I'm assuming old versions are unsupported now that this is merged?

@ghost
Copy link
Copy Markdown
Author

ghost commented Jan 8, 2018

@FanaHOVA It should still be possible to install the old chartjs -- you will see a deprecation message but installation should still succeed.
If you want to maintain types for an old version of the package on DefinitelyTyped it would also be possible to create a chart.js/v0 or chart.js/v1 directory containing those typings, similar to node/v0.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Revision needed This PR needs code changes before it can be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@FanaHOVA @typescript-bot @mhegazy @paulvanbrenk