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
Allow codebase to be checked by TypeScript compiler #7030
Conversation
592aa38
to
0b478ea
Compare
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 about all, but I think the changes to core.adapters.js
are good. Could extract that as separate PR.
0b478ea
to
fdf4397
Compare
5adf5ea
to
644bfe5
Compare
b3dcb5f
to
a99044e
Compare
f65f4ae
to
a485ab3
Compare
@anuti @FabienLavocat @KentarouTakeda @larrybahr @mernen @josefpaij @danmana @guillaume-ro-fr @archy-bold @braincore @frabnt @alexdor @mahnuh @Conrad777 @adripanico @wertzui @lekoaf @ElianCordoba @indigolain @Ricardo-Mello @rnicholus @mrjack88 @canoceto thanks for contributing to the Chart.js d.ts definitions on definitelytyped. We're now working on Chart.js 3 and exploring many new things. As part of that I've been playing around with TypeScript. I'm currently using TypeScript with the entire codebase as pure JavaScript using I'm not sure yet whether we'll merge this PR. I expect the main hesitation would be that it makes the build size larger. But if we import the types file then perhaps we don't need this PR. If any of you want to contribute to that I'd be interested to learn what that looks like. No promises yet that it'd definitely get merged, but I'm at least interested in learning more from folks with more experience in this area. |
a485ab3
to
b8552ad
Compare
I think moving the type defs out of DT and directly into the project is a net win for everyone. Additionally, it will make TS compliance for visible for all contributors, and changes can be immediately verified for Ts-related issues as part of the contribution. After 3.0 is complete w/ published type defs, a PR can be opened on DT to remove the types package. |
b8552ad
to
df2344b
Compare
If we move the types into this project, would this PR still be required? Would the types be published as part of the Chart.js package or as their own package? |
The types should be published as part of this package. Assuming the package.json file is updated to point to the |
I would assume similar to what was done with datalabels: chartjs/chartjs-plugin-datalabels@9e00265 |
I'm not sure how the types declaration file gets validated to see if it matches the js. It looks like it doesn't just automatically happen at compile time and you need to write tests for each type declaration or something? |
974425d
to
b5189a0
Compare
050c7a3
to
bf58662
Compare
6e2b6ff
to
a6b1606
Compare
@@ -251,7 +255,7 @@ export function _steppedLineTo(ctx, previous, target, flip, mode) { | |||
const midpoint = (previous.x + target.x) / 2.0; | |||
ctx.lineTo(midpoint, previous.y); | |||
ctx.lineTo(midpoint, target.y); | |||
} else if (mode === 'after' ^ flip) { | |||
} else if (mode === 'after' !== !!flip) { |
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.
oh, the horror.
ctx.closePath(); | ||
} else { | ||
interpolatedLineTo(ctx, target, end, property); | ||
} | ||
|
||
loop &= target.pathSegment(ctx, tgt, {move: loop, reverse: true}); | ||
const targetLoop = !!target.pathSegment(ctx, tgt, {move: lineLoop, reverse: true}); |
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.
Just FYI:
Could have actually done it like this (just reverse your initial try, so the short-circuiting does not have any effect):
loop = target.pathSegment(ctx, tgt, {move: loop, reverse: true}) && loop;
me.doughnutMode = false; | ||
this.doughnutMode = false; | ||
|
||
this.chart = config.chart; |
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.
I wonder if disabling the ts compiler option strictPropertyInitialization
would allow omitting these.
Enable strict checking of property initialization in classes.
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.
Hmmm. It sure sounds like it would. But I tried it and it didn't have any effect and the docs also say it's set to false
by default
a6b1606
to
ac9282d
Compare
ac9282d
to
564fcca
Compare
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.
Couple of things that are not that pleasing to me, but I'll forget about those quite soon 🤣
const me = this; | ||
const segments = me.segments; | ||
const ilen = segments.length; | ||
const segmentMethod = _getSegmentMethod(me); | ||
let loop = me._loop; | ||
for (let i = 0; i < ilen; ++i) { | ||
loop &= segmentMethod(ctx, me, segments[i], params); | ||
loop &= segmentMethod(ctx, me, segments[i]); |
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.
I'm wondering why &=
is allowed here?
Lets merge this? |
I think those errors I got in #7069 will surface on merge. @benmccann can you do a (local) rebase and see? |
I'd seen those errors when generating types as well, but shouldn't affect this PR |
DatasetController was converted to class, which makes these surface. |
This PR is already rebased against the latest from |
Right, I had probably a old copy. Good to go then! |
I believe it's a mistake to only delete the metaset if it has a valid controller; see f191f2f for where this behavior was introduced. This is a minimal fix for chartjs#9653; as discussed there, it may also be worth updating `updateHoverStyle`. As of chartjs#7030, `this._metasets` should always be defined, so checking whether it's undefined is no longer necessary.
* Fix cleaning up metasets I believe it's a mistake to only delete the metaset if it has a valid controller; see f191f2f for where this behavior was introduced. This is a minimal fix for #9653; as discussed there, it may also be worth updating `updateHoverStyle`. As of #7030, `this._metasets` should always be defined, so checking whether it's undefined is no longer necessary. * Add a test covering metaset behavior * Add a regression test for #9653; fix `toHaveSize` usage * Fix test failure
I think the main benefits of this would be better documentation for our users. Mainly it would make it clear what properties are available on a class and make sure there are no errors in the types, etc.
The main difference is that it requires you to define all properties in the class constructor. That might not minify as well, but it is a lot easier to tell what properties are available on a class both for developers looking at the code and users looking at the jsdoc. It's also maybe better performance because all properties are created upfront? I'm not too sure
I also expect this would make it much easier to use Chart.js in a TypeScript project. Especially if we went a step further and actually converted the code to TypeScript
This fixes all 1110 errors reported by TypeScript.
A few main annoyances:
this
to be used directly in constructorsnpm
users by adding tree shaking in v3update
methods and just reconstructing some objects. There's no reason to keep the old object and essentially recreate it inupdate
much of the timeOverall, I think this is a win. It is a bit more code, but I think it's worth the tradeoff for more maintainable code and better documentation. It's really nice to be able to easily tell what all the public properties are on a class, which you couldn't do before and I think suggests some possible additional cleanups. It's also uncovered a pretty good handful of issues:
generate
is calledticksLength
andborderValue
on an array, which seemed pretty weird. I'm changing in this PRI added a
tsconfig.json
file, so you can simply run typescript with:Or generate the docs with: