Allow codebase to be checked by TypeScript compiler #7030

benmccann · 2020-01-30T01:31:57Z

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:

TypeScript requires this to be used directly in constructors
TypeScript requires all properties be defined in constructors, which will increase download size
- We may be able to reduce the impact for npm users by adding tree shaking in v3
- We may be able to reduce the impact by removing update methods and just reconstructing some objects. There's no reason to keep the old object and essentially recreate it in update much of the time

Overall, 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:

Probably the most severe issue it's found is Fix isRadial is always false #7037 followed by Fix bug in opacity handling #7047
Several places where extra parameters are being passed. E.g. helpers.rtl.restoreTextDirection being called with extra parameter #7035, handleMaxPadding being called with extra parameter #7036, and in this PR in the time scale when generateis called
Numerous JSDoc errors (Fix some JSDoc errors #7026 & JSDoc and other cleanup #7057)
Setting the properties ticksLength and borderValue on an array, which seemed pretty weird. I'm changing in this PR
Unused variable (Remove unused variable #7031)

I added a tsconfig.json file, so you can simply run typescript with:

npx tsc

Or generate the docs with:

npx typedoc

kurkle

Not sure about all, but I think the changes to core.adapters.js are good. Could extract that as separate PR.

src/core/core.scale.js

src/core/core.animator.js

src/plugins/plugin.legend.js

tsconfig.json

benmccann · 2020-02-03T04:46:55Z

@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 allowJs and checkJs. It's helped me find a few bugs and clean up our JSDoc. I'm not sure whether this PR is the best way forward to start or whether it'd be to try importing the types file into this repo and updating it in line with the migration guide or changes in this file. This is my first time using TypeScript and I've taken this about as far as I have the knowledge and time to do.

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.

rnicholus · 2020-02-03T14:43:52Z

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.

benmccann · 2020-02-03T15:25:41Z

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?

rnicholus · 2020-02-03T15:36:39Z

The types should be published as part of this package. Assuming the package.json file is updated to point to the .d.ts file w/ the type defs (via "types" prop), that should be all that is needed for TS to pick up the defs for any users of Chart.js.

kurkle · 2020-02-03T15:39:45Z

I would assume similar to what was done with datalabels: chartjs/chartjs-plugin-datalabels@9e00265

benmccann · 2020-02-03T17:16:07Z

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?

src/core/core.ticks.js

src/core/core.scale.js

src/elements/element.line.js

kurkle · 2020-02-06T19:22:27Z

src/helpers/helpers.canvas.js

 		ctx.lineTo(midpoint, previous.y);
 		ctx.lineTo(midpoint, target.y);
-	} else if (mode === 'after' ^ flip) {
+	} else if (mode === 'after' !== !!flip) {


oh, the horror.

kurkle · 2020-02-06T19:38:43Z

src/plugins/plugin.filler.js

 		}

-		loop &= target.pathSegment(ctx, tgt, {move: loop, reverse: true});
+		const targetLoop = !!target.pathSegment(ctx, tgt, {move: lineLoop, reverse: true});


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;

kurkle · 2020-02-06T19:42:19Z

src/plugins/plugin.legend.js

-		me.doughnutMode = false;
+		this.doughnutMode = false;
+
+		this.chart = config.chart;


I wonder if disabling the ts compiler option strictPropertyInitialization would allow omitting these.

Enable strict checking of property initialization in classes.

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

kurkle

Couple of things that are not that pleasing to me, but I'll forget about those quite soon 🤣

kurkle · 2020-02-07T18:01:19Z

src/elements/element.line.js

 		let loop = me._loop;
 		for (let i = 0; i < ilen; ++i) {
-			loop &= segmentMethod(ctx, me, segments[i], params);
+			loop &= segmentMethod(ctx, me, segments[i]);


I'm wondering why &= is allowed here?

etimberg · 2020-02-07T20:03:55Z

Lets merge this?

kurkle · 2020-02-07T20:06:54Z

I think those errors I got in #7069 will surface on merge. @benmccann can you do a (local) rebase and see?

benmccann · 2020-02-07T20:12:17Z

I'd seen those errors when generating types as well, but shouldn't affect this PR

kurkle · 2020-02-07T20:15:05Z

DatasetController was converted to class, which makes these surface.

benmccann · 2020-02-07T21:01:42Z

This PR is already rebased against the latest from master

kurkle · 2020-02-07T21:22:53Z

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

benmccann force-pushed the typescript-core.scale branch from 592aa38 to 0b478ea Compare January 30, 2020 04:30

benmccann changed the title ~~Allow core.scale to be checked by TypeScript compiler~~ Allow scales to be checked by TypeScript compiler Jan 30, 2020

kurkle reviewed Jan 30, 2020

View reviewed changes

benmccann mentioned this pull request Jan 30, 2020

Convert the DateAdapter to ES6 #7032

Merged

benmccann added the type: infrastructure label Jan 30, 2020

benmccann force-pushed the typescript-core.scale branch from 0b478ea to fdf4397 Compare January 31, 2020 00:33

etimberg previously approved these changes Jan 31, 2020

View reviewed changes

src/core/core.scale.js Show resolved Hide resolved

benmccann marked this pull request as ready for review January 31, 2020 00:41

benmccann dismissed etimberg’s stale review via 9701fac January 31, 2020 23:57

benmccann force-pushed the typescript-core.scale branch 3 times, most recently from 5adf5ea to 644bfe5 Compare February 1, 2020 04:43

kurkle reviewed Feb 1, 2020

View reviewed changes

src/core/core.animator.js Outdated Show resolved Hide resolved

src/plugins/plugin.legend.js Show resolved Hide resolved

tsconfig.json Outdated Show resolved Hide resolved

benmccann force-pushed the typescript-core.scale branch 2 times, most recently from b3dcb5f to a99044e Compare February 2, 2020 22:52

benmccann changed the title ~~Allow scales to be checked by TypeScript compiler~~ Allow codebase to be checked by TypeScript compiler Feb 2, 2020

benmccann force-pushed the typescript-core.scale branch 4 times, most recently from f65f4ae to a485ab3 Compare February 3, 2020 04:17

benmccann force-pushed the typescript-core.scale branch from a485ab3 to b8552ad Compare February 3, 2020 14:24

benmccann force-pushed the typescript-core.scale branch from b8552ad to df2344b Compare February 3, 2020 14:52

benmccann force-pushed the typescript-core.scale branch 2 times, most recently from 974425d to b5189a0 Compare February 3, 2020 22:43

benmccann force-pushed the typescript-core.scale branch 3 times, most recently from 050c7a3 to bf58662 Compare February 6, 2020 14:30

etimberg reviewed Feb 6, 2020

View reviewed changes

src/core/core.ticks.js Show resolved Hide resolved

benmccann force-pushed the typescript-core.scale branch 4 times, most recently from 6e2b6ff to a6b1606 Compare February 6, 2020 18:36

kurkle reviewed Feb 6, 2020

View reviewed changes

benmccann force-pushed the typescript-core.scale branch from a6b1606 to ac9282d Compare February 6, 2020 20:03

benmccann mentioned this pull request Feb 6, 2020

core.scale cleanup #7067

Merged

etimberg previously approved these changes Feb 6, 2020

View reviewed changes

Fix errors reported by TypeScript compiler

564fcca

benmccann dismissed etimberg’s stale review via 564fcca February 6, 2020 23:40

benmccann force-pushed the typescript-core.scale branch from ac9282d to 564fcca Compare February 6, 2020 23:40

kurkle approved these changes Feb 7, 2020

View reviewed changes

kurkle mentioned this pull request Feb 7, 2020

Generate types #7069

Closed

etimberg approved these changes Feb 7, 2020

View reviewed changes

etimberg merged commit 795c86e into chartjs:master Feb 7, 2020

etimberg linked an issue Feb 9, 2020 that may be closed by this pull request

handleMaxPadding being called with extra parameter #7036

Closed

kurkle mentioned this pull request Feb 11, 2020

Types (=docs) for IEvent and IPlugin #7090

Closed

joshkel mentioned this pull request Sep 16, 2021

Fix cleaning up metasets #9656

Merged

Allow codebase to be checked by TypeScript compiler #7030

Allow codebase to be checked by TypeScript compiler #7030

Uh oh!

Conversation

benmccann commented Jan 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kurkle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benmccann commented Feb 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rnicholus commented Feb 3, 2020

Uh oh!

benmccann commented Feb 3, 2020

Uh oh!

rnicholus commented Feb 3, 2020

Uh oh!

kurkle commented Feb 3, 2020

Uh oh!

benmccann commented Feb 3, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kurkle Feb 6, 2020

Choose a reason for hiding this comment

Uh oh!

kurkle Feb 6, 2020

Choose a reason for hiding this comment

Uh oh!

kurkle Feb 6, 2020

Choose a reason for hiding this comment

Uh oh!

benmccann Feb 6, 2020

Choose a reason for hiding this comment

Uh oh!

kurkle left a comment

Choose a reason for hiding this comment

Uh oh!

kurkle Feb 7, 2020

Choose a reason for hiding this comment

Uh oh!

etimberg commented Feb 7, 2020

Uh oh!

kurkle commented Feb 7, 2020

Uh oh!

benmccann commented Feb 7, 2020

Uh oh!

kurkle commented Feb 7, 2020

Uh oh!

benmccann commented Feb 7, 2020

Uh oh!

kurkle commented Feb 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

benmccann commented Jan 30, 2020 •

edited

Loading

benmccann commented Feb 3, 2020 •

edited

Loading