-
Notifications
You must be signed in to change notification settings - Fork 30
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
Add type definitions #166
Add type definitions #166
Conversation
@Enteleform may I ask you to review this PR? |
@akx hi! May I ask you to review this PR? |
@ai hi! May I ask you to review this PR? |
@@ -0,0 +1,3 @@ | |||
import type { Color } from './common'; | |||
|
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 noticed that when in the file, for example in displayable.js
, when there is an import which looks like import converter from './converter.js';
because of .js
in the path, at least in WebStorm not showing the type hint and autocomplition is not working.
src/_prepare.d.ts
Outdated
@@ -0,0 +1,16 @@ | |||
import parse from './parse.js'; |
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.
import parse from './parse.js'; | |
import parse from './parse'; |
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.
ESM requires a full path with extension
src/_prepare.d.ts
Outdated
): Color; | ||
declare function prepare<M extends Mode>( | ||
color: Omit<Color, 'mode'> & { mode?: M }, | ||
mode: undefined |
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.
Maybe this parameter should just be elided?
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've updated the 'prepare' function type and the examples 5b0f7b3#diff-072ed26e63b74da148449088d6e78e3b145592e85a93e25d2aea3f7a682d804eR82
What do you think about them?
My goal here was to express in TS the behavior of the 'prepare' function as closer as possible.
@akx thank you for review! |
src/filter.d.ts
Outdated
@@ -0,0 +1,11 @@ | |||
import type { ColorToColor, Mode } from './common'; | |||
|
|||
type Filter = (amt?: number, mode?: Mode) => ColorToColor; |
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.
According to the documentation
The filters were designed for RGB colors, so the mode parameter is expected to be rgb or lrgb.
Should a type of the mode
parameter looks like 'rgb' | 'lrgb'
?
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 think the question above should be asked to the creator of the library? @danburzo what do you think?
@@ -29,3 +29,23 @@ tape('gamma transfer function', t => { | |||
t.equal(formatHex(brighten('#cc0033')), '#ff0070'); | |||
t.end(); | |||
}); | |||
|
|||
tape( |
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.
@danburzo hi!
Before this commit: 7c0ff5a the following code throws an error:
mapper(mapTransferGamma(2.2), null)(
'color(--okhsv 29.2 0.9 0.9)'
)
What do you think about this bugfix?
}); | ||
|
||
// THROWS Type 'Rgb' is missing the following properties from type 'Hsl': h, s, l | ||
const case_3_expect_error: Hsl = doubler({ |
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.
@ai I added tests for .d.ts
files.
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 use this typing test system in all my open source.
For instance, in PostCSS https://github.com/postcss/postcss/blob/main/test/errors.ts
@danburzo what else do we need here? I really need TS support to use your awesome |
@ai @bijela-gora Sorry for the delay, I hope to be able to review this PR soon. |
@ai in the side branch, I removed the
|
@akx FYI ☝️ |
@bijela-gora we need to add types for |
@ai do you know how it can be done? The support of I also found that it is possible to import tree shakeable function using |
I can install TS 4.7 beta. But it doesn’t work too. You can try on your own:
|
@ai thank you I will add Also I have found that I configure tsconfig in the wrong way, and I need to fix many type definitions. It will take 1 or 2 days. |
@bijela-gora ping me when you will be able to run it locally (with new |
@ai ping :-) Use Here is the patch to the oklch-picker with updated types. |
@bijela-gora can you send this patch (with |
Here it is: evilmartians/oklch-picker#35 |
@danburzo I can prove that types works. We tested and polished types in evilmartians/oklch-picker#35 |
@danburzo can we merge it please? TS support is very critical for modern big applications. |
I'd also very much like to have this merged :) |
Dear @bijela-gora & others in this thread, I have failed to answer when pinged, but I think the situation deserves a status update despite my reluctance before we hit the 1-year mark. First of all, thank you for what looks to me like an immense amount of work for this PR, and what looks like a successful integration in the excellent evilmartians/oklch-picker ❤️ However, this is a massive PR that turned out to include a lot of files and duplicated code, which I fear is too much of a maintenance burden to take on all at once. (Please bear in mind that, while I am open to the idea of better supporting Typescript users of the library, I am not a TypeScript user myself.) Therefore, as much as it pains me to say, I will not be able to merge it in this form, at least for the time being. I need to get at least a bit familiar with the technology before I can commit to maintaining it going forward. I am currently exploring how to generate Declaration files from JavaScript via JSDoc, which I think is the only feasible way forward for types in Culori. I will be posting ideas/progress in #128. I'm sorry, I know this is a disappointing answer... Once I have a workable solution for generating types, I hope to find a way to integrate the work you've already done here. |
@danburzo don’t worry. OKLCH color picker temporary moved to local types evilmartians/oklch-picker@3dd6cbf |
@ai @bijela-gora Given the incredible amount of work already done here, could it make sense to contribute these type definitions to DefinitelyTyped, as It would be a nice stopgap solution for the TS users out there until @danburzo can find a long-term solution for maintainable first-party typings. |
I'm going to create a PR to DefinitelyTyped. |
Can this please be updated to include newer API methods like |
Hi all :-)
This MR is related to #128
I'm not the user of this library. I come on call of @ai here https://cultofmartians.com/tasks/culori-types.html
What has been done in the scope of this PR so far:
npm link
that type defs were exported correctly and can be used in the user project.The advantage of the approach when declaration files is in the same dir as javascript files is that the types are immediately available to other parts of the code base, for example in tests. Will this advantage be helpful for the maintainers?
May I get feedback at this point?