[types] Add type definitions from DefinitelyTyped/DefinitelyTyped#62510 #221
Conversation
Moves types from the original DefinitelyTyped PR and adds some dependencies and workflows to test them. These changes are super broken on my machine right now and I can't figure out why, but I'll try to get it fixed.
|
Wow, that is a very substantial amount of work, thank you so much @MysteryBlokHed! I'm a bit worried I lack the TS experience to review this properly, perhaps someone else could take a look when it's ready? @ai @jgerigmeyer would that be of interest to you? |
Yes, I'd be happy to take a look at this! I should be able to do that in the next few days. 👍 |
|
I think most of the API has types now. I just need to figure out the best way to add types for the space accessors |
There was a problem hiding this comment.
This is so helpful -- thanks @MysteryBlokHed! 🎉
I wasn't able to get dtslint running locally without some further changes -- submitted in https://github.com/MysteryBlokHed/color.js/pull/1
I pulled this in to a personal TypeScript project that uses colorjs.io (replacing some type stubs I'd written there), and there were only a few places where it seems these types were slightly incorrect -- I submitted a first pass at some changes in https://github.com/MysteryBlokHed/color.js/pull/2
* Initial round of types review * Address review
|
@jgerigmeyer thank you so much! I just added you as a collaborator. @MysteryBlokHed @jgerigmeyer is this ready to merge? |
| "src/", | ||
| "types/dist/", | ||
| "types/src/", | ||
| "types/index.d.ts" |
There was a problem hiding this comment.
Why did we choose this structure over src/types and dist/types? Is that the convention? (no problem if that's the case, just thought I'd ask)
There was a problem hiding this comment.
I just kept this structure when I copied the types over from DefinitelyTyped, but I think doing it this way also makes testing easier since everything's in the same place
| - uses: actions/checkout@v3 | ||
| - uses: actions/setup-node@v3 | ||
| with: | ||
| node-version: "12" |
There was a problem hiding this comment.
Isn't 12 a bit old at this point?
There was a problem hiding this comment.
I think I chose 12 because the Babel config specifies version <=12, but it could be made more recent
| @@ -1,4 +1,4 @@ | |||
| dist/ | |||
| /dist/ | |||
There was a problem hiding this comment.
Adding the leading slash means that Git will only ignore the dist folder in the root of the project. Without this change, the folder types/dist is also ignored
|
I had a quick look and left some questions. I see there is also a new |
|
I'm 99% sure that we'll discover small issues once people start actually using this with TypeScript, but IMO this is already a significant improvement and is ready to merge. 🎉 It would be much easier to maintain these types going forward if the codebase itself were re-written in TypeScript, but I understand that's not a simple change and wouldn't have to happen right away regardless. |
The tests in the A mention of the reason for the two directories could be mentioned in the README or CONTRIBUTING files maybe? I could make a change when I'm free in a few hours |
Ok, I plan to rebase and merge soon, so if there's anything you want to squash in the commit log, now's the time.
I know, but I don't want to tie the source to a build tool, it's nice that right now things like this work in a browser: |
I totally missed they were in |
I'm not particular about a "clean" git history, so I'll defer to @MysteryBlokHed on whether to squash before merging. |
|
Rebasing seems fine and I don't think there's anything else that needs changing right now |
|
Merged, thank you both! |
|
Was this shipped in latest release ? |
|
No, it will be in the next release (likely v0.4.1) |
Closes #119
Moves types from the original DefinitelyTyped PR and adds some dependencies and workflows to test them.
This PR is still very much a work in progress, and I'm pretty sure there are some parts of the API that I haven't typed yet.