-
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
[Feature Request] TypeScript Definitions #128
Comments
As mentioned around that comment, there's a partial .d.ts in https://github.com/akx/gradient/blob/master/src/culori.d.ts that could work as a starting point. |
By the way - something I noticed e.g. https://github.com/syntax-tree/mdast-util-from-markdown does is use TypeScript's JSDoc support to allow type annotations, inference and declaration file creation in plain-JS files: syntax-tree/mdast-util-from-markdown@052ad82. Would you be amenable to something like that, @danburzo? I could look into how that might work here :) |
JSDoc syntax looks like very limited. For instance, export type Color = string
| { mode: 'rgb', r: number, g: number; b: number }
| { mode: 'lch', l: number, c: number, h: number } I am not sure that it is easy to define such types in JSDoc. At least it will not be very readable and maintainable. |
It's tedious but possible to do this with JSDoc and type definitions can go into separate files. /**
* @typedef {Object} RgbMode
* @property {String} mode 'rgb' Color mode name
* @property {Number} r Red component
* @property {Number} g Green component
* @property {Number} b Blue component
* /
/**
* @typedef {Object} LchMode
* @property {String} mode 'lch' Color mode name
* @property {Number} l Luminance component
* @property {Number} c Chroma component
* @property {Number} h Hue component
* /
/**
* @type {String | RgbMode | LchMode } Color
*/
/**
* @param String color
* @return Color
*/
const parseHwb = color => { ... } That's just two colour mode types hacked together (can't recall how to declare default values) and it'll be very, very elaborate to annotate each function inline unlike declaring types and interfaces in a separate |
@WebMechanic hi! The PR that adds TS types is ready #166 Do you think it is possible to do the same with JSDoc? Does it make sense? |
Hello @bijela-gora, The examples I was giving are very basic. There is much more, but JSDoc can't replace the granular details of (real) type definitions and unlike them it doesn't infer information from existing code. It "knows" what you add on top inside the code comments and the IDE will present what you write. Most IDEs and code editors can leverage JSDoc contents to give similar feedback like type definitions, such as squiggly lines if you mistype a param or pass the wrong data type to a function. The TSC certainly doesn't give a toss about them and won't validate anything. So, can it do the same? Does it make sense? Thanks for your work! |
Despite I have added descriptions to functions, classes, methods before when it seemed important, аfter your comment, I feel that I will be more attentive to the documentation for developers, and will take this use case into account. Thank you! |
FYI you can always sprinkle some JSDoc fairy dust onto your .d.ts as you see fit. /**
* @param [amt] Amount of filter applied
* @param [mode] The mode parameter is expected to be 'rgb' or 'lrgb', because filters were designed for RGB colors.
*/
type Filter = (amt?: number, mode?: Mode) => ColorToColor; In this case the parameter data types are taken from the declaration (no need to repeat them then) and their decription from the JSDoc. |
@bijela-gora works great! Thanks. |
Any updates on the issue? https://www.npmjs.com/package/@types/culori is fine, but misses a few API methods e.g. |
Would the maintainers now be open to a PR that updates the library to be written in TypeScript (in case opinions may have changed in the 3 years since this issue was opened)? That way it ships its own definitions. I’d be happy to open a PR if desired 🙂 We could also update the test suite to use Vitest, which is not only spec-compliant and supports TypeScript, it’s fast, too (only in case test runner is a potential blocker to migrating to TypeScript) |
Hi @drwpow, my thoughts on the matter can be found here, and it’s still the case that migrating to TS will introduce a maintenance burden that I cannot sustain at the moment, in addition to aesthetic concerns for the library (the intentional removing of as much tooling / dependencies as possible). The previous PR made apparent just how much new API design surface a migration entails (naming things, etc.). If incremental adoption of a JSDoc-to-types sort of thing were feasible, that would be a good compromise I think. |
@danburzo thanks for replying! And thanks for re-posting that more recent response; I hadn’t seen that PR/discussion/context. That PR is actually superior than the JSDoc approach, because when shipping a library to consumers you really need those That’s why This issue also is important because the DefinitelyTyped solution of All that said, I completely respect your decision, and it’s important to set healthy boundaries on open-source projects. I just selfishly want to beg you to reconsider 😄. As someone that’s used TS for years, I would strongly disagree it’s a maintenance burden—the opposite, in fact. It saves you from having to write so many tests, and the tooling is so mature now it’s trivial to compile. It is a learning curve, but hopefully a short one with the right guidance. And I (and I’m sure others) can help lighten the load so that the migration isn’t scary, or “too much,” and the end result still looks, feels, and operates like the exact-same code. |
Thank you for that perspective, and for the kind words. |
Just following up on this comment by @danburzo
Yes please! That would be very helpful and make the library much easier to use via IntelliSense.
The text was updated successfully, but these errors were encountered: