Skip to content
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

Closed
wants to merge 145 commits into from
Closed

Add type definitions #166

wants to merge 145 commits into from

Conversation

bijela-gora
Copy link
Contributor

@bijela-gora bijela-gora commented Apr 5, 2022

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:

  • types have been added for some exported functions
  • it was checked with 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?

@bijela-gora bijela-gora marked this pull request as ready for review April 11, 2022 21:05
@bijela-gora
Copy link
Contributor Author

@Enteleform may I ask you to review this PR?

@bijela-gora
Copy link
Contributor Author

@akx hi! May I ask you to review this PR?

@bijela-gora
Copy link
Contributor Author

@ai hi! May I ask you to review this PR?

@bijela-gora bijela-gora changed the title Add some types Add type definitions Apr 11, 2022
@@ -0,0 +1,3 @@
import type { Color } from './common';

Copy link
Contributor Author

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.

copy-types.js Outdated Show resolved Hide resolved
src/_prepare.d.ts Show resolved Hide resolved
@@ -0,0 +1,16 @@
import parse from './parse.js';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import parse from './parse.js';
import parse from './parse';

Copy link

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

): Color;
declare function prepare<M extends Mode>(
color: Omit<Color, 'mode'> & { mode?: M },
mode: undefined
Copy link
Contributor

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?

Copy link
Contributor Author

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.

src/a98/types.d.ts Outdated Show resolved Hide resolved
src/average.d.ts Outdated Show resolved Hide resolved
src/clamp.d.ts Outdated Show resolved Hide resolved
src/clamp.d.ts Outdated Show resolved Hide resolved
src/common.d.ts Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
@bijela-gora
Copy link
Contributor Author

@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;
Copy link
Contributor Author

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' ?

Copy link
Contributor Author

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(
Copy link
Contributor Author

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({
Copy link
Contributor Author

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.

Copy link

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

@ai
Copy link

ai commented Apr 24, 2022

@danburzo what else do we need here?

I really need TS support to use your awesome culori in my project.

@danburzo
Copy link
Collaborator

@ai @bijela-gora Sorry for the delay, I hope to be able to review this PR soon.

@bijela-gora
Copy link
Contributor Author

bijela-gora commented Apr 28, 2022

I really need TS support to use your awesome culori in my project.

@ai @danburzo I'm here in case you found some inconsistency between documentation and type definitions, or in case some types is not convenient enough.

@bijela-gora
Copy link
Contributor Author

bijela-gora commented May 7, 2022

@ai in the side branch, I removed the prepare npm script from the list, and now you can install the culori package from GitHub as follows:

pnpm install github:bijela-gora/culori#7fa2b01940ee6c0793455c34872d5cef5d837b9e

@bijela-gora
Copy link
Contributor Author

@akx FYI ☝️

@ai
Copy link

ai commented May 9, 2022

@bijela-gora we need to add types for 'culori/fn' import (after it, I will be able to continue testing types)

@bijela-gora
Copy link
Contributor Author

@ai do you know how it can be done?

The support of package.json Exports will be added in TS 4.7

I also found that it is possible to import tree shakeable function using
import { parse } from 'culori/src/index-fn.js';

@ai
Copy link

ai commented May 10, 2022

The support of package.json Exports will be added in TS 4.7

I can install TS 4.7 beta. But it doesn’t work too.

You can try on your own:

  1. Repo https://github.com/evilmartians/oklch-picker
  2. File which use culori https://github.com/evilmartians/oklch-picker/blob/main/lib/colors.ts

@bijela-gora
Copy link
Contributor Author

bijela-gora commented May 10, 2022

@ai thank you

I will add fn.d.ts in the project root folder with export * from './src/index-fn'; content and this should solve the issue with 'culori/fn'.

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.

@ai
Copy link

ai commented May 10, 2022

@bijela-gora ping me when you will be able to run it locally (with new pnpm add command) and I will test and check types too

@bijela-gora
Copy link
Contributor Author

@ai ping :-)

Use pnpm remove culori && pnpm add github:bijela-gora/culori#16bcbc35 to update the package in project.

Here is the patch to the oklch-picker with updated types.

updated-types.zip

@ai
Copy link

ai commented May 13, 2022

@bijela-gora can you send this patch (with package.json and pnpm-lock.yml changes) as a PR to save your name in the OKLCH picker history?

@bijela-gora
Copy link
Contributor Author

Here it is: evilmartians/oklch-picker#35

@ai
Copy link

ai commented May 17, 2022

@danburzo I can prove that types works.

We tested and polished types in evilmartians/oklch-picker#35

@ai
Copy link

ai commented Jun 25, 2022

@danburzo can we merge it please? TS support is very critical for modern big applications.

@TimJentzsch
Copy link

I'd also very much like to have this merged :)

@danburzo
Copy link
Collaborator

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.

@ai
Copy link

ai commented Feb 20, 2023

@danburzo don’t worry. OKLCH color picker temporary moved to local types evilmartians/oklch-picker@3dd6cbf

@lordofthelake
Copy link

@ai @bijela-gora Given the incredible amount of work already done here, could it make sense to contribute these type definitions to DefinitelyTyped, as @types/culori?

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.

@bijela-gora
Copy link
Contributor Author

@danburzo @lordofthelake

I'm going to create a PR to DefinitelyTyped.

@bijela-gora
Copy link
Contributor Author

@lordofthelake

https://www.npmjs.com/package/@types/culori

@swernerx
Copy link

swernerx commented Mar 5, 2024

@lordofthelake

https://www.npmjs.com/package/@types/culori

Can this please be updated to include newer API methods like toGamut()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants