color-js / color.js

Color conversion & manipulation library by the editors of the CSS Color specifications
https://colorjs.io
MIT License
1.81k stars 80 forks source link

[types] Migrate to JSDoc #540

Closed MysteryBlokHed closed 4 weeks ago

MysteryBlokHed commented 1 month ago

Closes #499

This PR is the same as #528, but the source branch is now on the main color.js repo to make direct modifications easier.


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:

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[bot] commented 1 month ago

Deploy Preview for colorjs failed. Why did it fail? ā†’

Name Link
Latest commit ac5004e77ecf879079c32e4f7c0345d7d580512c
Latest deploy log https://app.netlify.com/sites/colorjs/deploys/6663a38160278e0008051906
MysteryBlokHed commented 1 month ago

Should the release draft for v0.6.0 be updated to point to just this PR, or to point to both the original and this one? This doesn't have any of the discussion from #528, if that's something we care about for the release notes

LeaVerou commented 1 month ago

Should the release draft for v0.6.0 be updated to point to just this PR, or to point to both the original and this one? This doesn't have any of the discussion from #528, if that's something we care about for the release notes

Not sure, would love more opinions. We could mention both, or we could rely on the fact that this links to #528. Iā€™m fine either way.

MysteryBlokHed commented 1 month ago

LGTM from a quick look. I would lave liked more locality for some of the types.js stuff, but we can explore later, it's imperative to merge this ASAP.

I put a lot of things in the types.d.ts because it made it a lot faster to just copy+paste from the old definitions files, but I agree that it makes sense to move definitions to their own relevant files (later).

And sorry about the formatting šŸ˜… I've turned off that extension for this repo

LeaVerou commented 1 month ago

So is there any blocker remaining? Shall we merge and let it ride the trains?

MysteryBlokHed commented 1 month ago

So is there any blocker remaining? Shall we merge and let it ride the trains?

I think we're all good now. dtslint tests are still failing, but most errors seem to be due to the tests being out of date with the API rather than actual problems that need fixing.

MysteryBlokHed commented 1 month ago

I or whoever else can merge as long as nobody else has any concerns

LeaVerou commented 4 weeks ago

Ship it!

Edit: I was too impatient šŸ˜