[types] Migrate to JSDoc

[MysteryBlokHed]

Closes #499

This PR migrates most .d.ts type definitions to JSDoc type annotations instead. This is done in an effort to keep code documentation and implementation in the same place.

Here's a checklist with tasks that I can think of so far:

• Files directly in src/ converted
• Files in subfolders of src/ converted (eg. deltaE/)
• Typing tests fixed to work with JSDoc annotations (semi-complete? runs just fine but there are type errors)

There are a few files that are currently still using .d.ts instead of JSDoc due to their complexity. Namely: color.d.ts, hooks.d.ts, index.d.ts, and space.d.ts.

Since the typings tests are currently not working, there's a chance that I've missed some bugs while doing this conversion. Once I find a way to get dtslint to run with JSDoc types, it'll be easier to spot any mistakes.

[netlify]

✅ Deploy Preview for colorjs ready!

Name	Link
🔨 Latest commit	e3bb307
🔍 Latest deploy log	https://app.netlify.com/sites/colorjs/deploys/6662291a7920df0008556369
😎 Deploy Preview	https://deploy-preview-528--colorjs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[MysteryBlokHed]

It seems like I had my editor's format-on-save using the wrong formatter, which unnecessarily messed with the code styling on a bunch of unrelated lines. I've undone some formatting changes, but I can go through and revert the other unintentional changes if desired.

[MysteryBlokHed]

One problem that I didn't think about regarding this change: for TypeScript to respect JSDoc types, you need to enable type checking for JS files. The consequences:

Some of these errors might just be from the files I haven't migrated yet, so I'll see how many clear up after that—hopefully to a manageable amount

[LeaVerou]

Thanks for working on this!!

For no-brainer things like line break at the end of files and space when defining literals inline, I'd be fine if you just committed them directly. Then it would be much easier to review the actual changes in this PR.

[LeaVerou]

One problem that I didn't think about regarding this change: for TypeScript to respect JSDoc types, you need to enable type checking for JS files. The consequences:

Some of these errors might just be from the files I haven't migrated yet, so I'll see how many clear up after that—hopefully to a manageable amount

Are these all? If so, that's not too bad.

[LeaVerou]

It may also be worth thinking, how could we make the transition gradually?

[MysteryBlokHed]

It may also be worth thinking, how could we make the transition gradually?

If there's any concern about how this will affect library consumers, I don't think anything will need to change—once the .d.ts files are removed, TypeScript should be able to detect the JSDoc comments and continue as usual.

Any .d.ts files that originally exported their own types have either been kept as .d.ts files alongside the JS source code, or have had their types exported like this:

/** @typedef {keyof typeof import("./index.js") extends `contrast${infer Alg}` ? Alg : string} Algorithms */

Which is equivalent to this:

color.js/types/src/contrast/index.d.ts

Lines 1 to 4 in ade2eb6

	export type Algorithms =
	keyof typeof import("./index.js") extends `contrast${infer Alg}`
	? Alg
	: string;

(The @typedef directive exports the defined type by default)

[LeaVerou]

If there's any concern about how this will affect library consumers,

Nah, it was more about making the work manageable for us, so that it doesn't need to be one gigantic PR by a single person.

But if you're willing to do that work, props to you!

[MysteryBlokHed]

Nah, it was more about making the work manageable for us, so that it doesn't need to be one gigantic PR by a single person.

Ah, that makes sense. I'd be willing to finish moving the types to JSDoc on my own through this PR, but it's probably worth discussing the best way to manage the other errors that have emerged (eg. is it better to try finding a way to suppress them all, or to actually make some of the implementation code more type-safe?)

I also personally wouldn't push these changes into a release until the typings tests are working again, since there's a chance that I've accidentally introduced a TypeScript bug here that won't be caught.

[LeaVerou]

Nah, it was more about making the work manageable for us, so that it doesn't need to be one gigantic PR by a single person.

Ah, that makes sense. I'd be willing to finish moving the types to JSDoc on my own through this PR, but it's probably worth discussing the best way to manage the other errors that have emerged (eg. is it better to try finding a way to suppress them all, or to actually make some of the implementation code more type-safe?)

From a quick look it looks like they should all be fixable by simply making the code more type-safe?

I also personally wouldn't push these changes into a release until the typings tests are working again, since there's a chance that I've accidentally introduced a TypeScript bug here that won't be caught.

Do we know how we can do that?

[MysteryBlokHed]

From a quick look it looks like they should all be fixable by simply making the code more type-safe?

That's true; it'd just be time-consuming. Although I am 100% willing to help if we do want to make the library's code type-safe.

Do we know how we can do that?

Right now, dtslint is just refusing to run the tests because of the type errors in the library itself. Once those are fixed, the tests will run as usual. Technically, it's doing its job exactly as it's meant to—it just means that I can't really test the JSDoc type definitions on their own until the type errors in the library are fixed.

[LeaVerou]

Gotcha. I think it will be easier to fix these once this PR is merged, so there's a bit of a chicken and egg problem.

[MysteryBlokHed]

We could just merge this once everything is re-typed with JSDoc, and then worry about fixing type errors with the library code after the fact? The only thing that's causing errors in this PR's current state is dtslint, but everything else should be fine.

I do think there are some files I still need to update (like the color spaces), but after that it should be ready.

[LeaVerou]

Sounds like a plan!

[MysteryBlokHed]

@LeaVerou When adding types for Format and Type, I changed the instance symbol to be an exported const instead of just a local let. That way, it can be referenced by other files in order to either index the format object, or to define types for it (like I did for the Format interface).

If that isn't wanted (i.e. the symbol should only be local), let me know and I'll undo that.

[LeaVerou]

@LeaVerou When adding types for Format and Type, I changed the instance symbol to be an exported const instead of just a local let. That way, it can be referenced by other files in order to either index the format object, or to define types for it (like I did for the Format interface).

If that isn't wanted (i.e. the symbol should only be local), let me know and I'll undo that.

It was meant to be private, as how instances are associated to their spec objects is an internal implementation detail (e.g. I could have used a WeakMap) but I don't feel strongly about that at all (and tend to favor extensibility over encapsulation as a general principle)

[MysteryBlokHed]

It was meant to be private, as how instances are associated to their spec objects is an internal implementation detail (e.g. I could have used a WeakMap) but I don't feel strongly about that at all (and tend to favor extensibility over encapsulation as a general principle)

I could also add an @internal note to the symbol's JSDoc. It doesn't actually do anything functional, but it indicates that it's not really a public part of the API

[LeaVerou]

Sounds good!

[MysteryBlokHed]

At this point I think all the exported members should be documented.

To start reducing type errors, I think the next step should be to also add type information to non-exported functions. That's where many errors come from (inferred any types), and it'll also provide type hints that might resolve some errors elsewhere in the project.

[MysteryBlokHed]

I can take a look at the merge conflicts later today

[jgerigmeyer]

I'll try to get to this review yet this week 👍

[LeaVerou]

Ugh, I forgot about this and just did a mega rename. How hard would it be to rebase? I can just revert my commit if it would be tricky.

[LeaVerou]

We should update this, not remove it! Same for the @returns below. Happy to leave it intact as well, so that someone else can update it.

[MysteryBlokHed]

I think I removed this because it was able to infer the return type, but we could still keep it there to document what exactly the return value represents

[LeaVerou]

@MysteryBlokHed any chance you could move this to a color.js branch? I was trying to review but all the formatting changes are making the diff a lot bigger than it needs to be and trying to revert them with suggestions is tedious and error-prone.

[MysteryBlokHed]

Ugh, I forgot about this and just did a mega rename. How hard would it be to rebase? I can just revert my commit if it would be tricky.

For the JS files, I think that if I just let Git auto-merge it should be able to resolve the conflicts on its own. For the typings files, it might try to recreate them all to show me the merge conflict, but I can just re-delete everything in that folder again.

I don’t think it’ll be too bad. I can try to merge it once I have access to an actual desktop (sending this reply from an iPad)

[MysteryBlokHed]

@MysteryBlokHed any chance you could move this to a color.js branch? I was trying to review but all the formatting changes are making the diff a lot bigger than it needs to be and trying to revert them with suggestions is tedious and error-prone.

Sure thing—I should be able to in a few hours.

[MysteryBlokHed]

If I'm not mistaken then you should also have write access to this branch on my fork (just through the way GitHub PRs work), but I can move it to color.js anyway if that makes it easier

[MysteryBlokHed]

Closing in favour of #540 (same thing, but the source branch isn't on my fork)

[LeaVerou]

If I'm not mistaken then you should also have write access to this branch on my fork (just through the way GitHub PRs work), but I can move it to color.js anyway if that makes it easier

Oh, good point. Oh well, too late now. Anyhow, in the future I think it's better to use branches in the repo itself.