adobe / leonardo

Generate colors based on a desired contrast ratio
http://www.leonardocolor.io
Apache License 2.0
1.94k stars 104 forks source link

`leonardo-contrast-colors`: Restore + enhance typescript support #207

Closed rsek closed 1 year ago

rsek commented 1 year ago

Resolves #143, resolves #175 :rocket:

Since I was already rooting around in the declaration file anyways, this PR also integrates content from the README into the TSDoc annotations to expose documentation in IDEs; I've sprinkled in some additional MDN links where relevant.

Something that crossed my mind as I was picking over the code: would there be any interest in a rewrite of leonardo-contrast-colors in TS? (Yes, a big ask, but FWIW, I'd be game).

My sense of things is that color spaces and interpolation can be esoteric to even experienced designers and developers. IMO one of the biggest benefits of TS is improving developer accessibility via tooling: hints in IDEs, generating API documentation from a "single source of truth", and so on (JSDoc can do a lot of this, but requires a fair bit of human intervention to produce useful TS typings).

To-do list

NateBaldwinDesign commented 1 year ago

@rsek Thank you so much for taking on those issues in this PR 💯 . I'm not familiar enough with Typescript to appropriately test these changes, but the value proposition mentioned in the description seems very much worthwhile.

What would the benefits be for rewriting the entire library in Typescript? What would that improve beyond the type definitions that you're already including here?

rsek commented 1 year ago

@rsek Thank you so much for taking on those issues in this PR 100 . I'm not familiar enough with Typescript to appropriately test these changes, but the value proposition mentioned in the description seems very much worthwhile.

What would the benefits be for rewriting the entire library in Typescript? What would that improve beyond the type definitions that you're already including here?

I think this article gives a good overview of the most common pros and cons with Typescript in general: https://www.altexsoft.com/blog/typescript-pros-and-cons/

One of the things got me thinking on this was this project's README, which is great - it's a pretty thorough description of the API. What struck me is that it's about as granular as Typescript gets in many cases: parameter x is a string, y is an optional boolean, so on.

The way I see it, a lot of Typescript is just explicit documentation that lives next to the code it's describing (and is machine-readable so tooling can take advantage of it and make people's lives easier with e.g. static type checks). From that perspective, Typescript migration would be a migration to a more structured rendering of that same information.

Aside from the benefit to people consuming the library, I think this improves maintainability as well. If the code is living right next to the documentation that describes it, then it's much easier for to maintain parity between the two, and for new contributors to parse out what's going on.

Other (better?) options

But with all that said -- I mentioned JSDoc earlier, which literally is a standard for inline documentation (that overlaps significantly with TS annotations). Now that I've thought it over more (code migrations are less appealing in the cold light of a February day, ha), I reckon that might be a friendlier path to achieving a similar outcome.

The end result of that would be eliminating a separate index.d.ts entirely, and instead include some (fairly verbose) annotations in the JS files themselves, which the TS servers in IDEs can then take advantage of.

The possible downside there is JSDoc vs. IDE handling of imported types, which can get weird. (Confession: I initially started this PR as JSDoc-based, but ran up against these issues and opted to keep the *.d.ts declaration file because I knew I could make that work. In hindsight, I probably should have kept that code around :facepalm:).

Takeaway

I think my next step here would be to draft a separate PR to explore a JSDoc-based approach further. Even if TS support there leaves something to be desired, it still results in a bunch of comments annotating the original code, which I don't think would be a bad thing, and would be a great starting point for a TS migration if one is later desired.

unzico commented 1 year ago

@NateBaldwinDesign Aside from the TypeScript discussion, can this PR please be merged? Right now it's not possible to use this library at all due to the issues fixed in this PR.

A new alpha release with this PR would be very much appreciated! :rocket:

NateBaldwinDesign commented 1 year ago

@GarthDB @lazd or @DmitryBaranovskiy able to do this?

lazd commented 1 year ago

@NateBaldwinDesign merged! Does this need to be manually released?

trevoring-okta commented 9 months ago

Hi, I can see that the version was bumped in main to 1.0.0-alpha.18 back in August: https://github.com/adobe/leonardo/commit/80ae5af90db4ecd47512fcb9c5ce8e3bf47f26f7

However, it doesn't look like the bumped package was ever actually released to npm, the latest there is still alpha.17: https://www.npmjs.com/package/@adobe/leonardo-contrast-colors?activeTab=versions

Was a decision made to not release the new version?