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

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

Merged
merged 19 commits into from Mar 11, 2023

Conversation

rsek
Copy link
Contributor

@rsek rsek commented Mar 1, 2023

Resolves #143, resolves #175 🚀

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

  • I have read the CONTRIBUTING document.
  • Integrate documentation from README as TSDoc annotations
  • Include some Typescript string literal templates for CSS colors, in order to clarify acceptable arguments used in various classes and functions. This is many lines of code, but shouldn't have any impact at runtime.
  • Test typings in an actual project
    • adjust package.json + file extensions: as package.json has "type": "module", the entire package is understood to use ES modules for *.js files, making the *.mjs extension redundant. using ES modules + *.mjs can cause problems with dependency resolution in some TS environments (e.g. ts-node: see ESM support: soliciting feedback TypeStrong/ts-node#1007 ).
  • This pull request is ready to merge.

@rsek rsek changed the title Update broken typescript typings Update broken Typescript declarations and improve their inline documentation Mar 2, 2023
@rsek rsek changed the title Update broken Typescript declarations and improve their inline documentation leonardo-contrast-colors: Fix broken index.d.ts and improve its inline documentation Mar 2, 2023
@rsek rsek marked this pull request as ready for review March 2, 2023 04:46
@rsek rsek changed the title leonardo-contrast-colors: Fix broken index.d.ts and improve its inline documentation leonardo-contrast-colors: Restore + enhance typescript support Mar 2, 2023
@NateBaldwinDesign
Copy link
Collaborator

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

rsek commented Mar 3, 2023

@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 🤦).

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
Copy link

unzico commented Mar 11, 2023

@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! 🚀

@NateBaldwinDesign
Copy link
Collaborator

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

@lazd lazd merged commit 4459610 into adobe:main Mar 11, 2023
1 check passed
@lazd
Copy link
Member

lazd commented Mar 11, 2023

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

@trevoring-okta
Copy link

Hi, I can see that the version was bumped in main to 1.0.0-alpha.18 back in August: 80ae5af

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?

thierryc pushed a commit to thierryc/leonardo that referenced this pull request Mar 29, 2024
…be#207)

* explicit types location

* initial commit

* rm extra whitespace

* add note for later

* verbose FIXMEs

* improve inline documentation

* adjust some inline documentation

* color string templates to provide some IDE hints

* adjust whitespace

* fix type union

* fix apca-w3 incorrectly marked as devdep

* rm unreferenced devdep

* correct import and entry paths

* "mjs" => "js" since package.json specifies "type"

* adjust test file glob

* add module declaration for chroma-js extension

* add missing oklab/oklch string types

* replace missing oklab/oklch colorspaces

* update functional notation to reflect std per MDN
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants