medialab / iwanthue

Colors for data scientists.
http://tools.medialab.sciences-po.fr/iwanthue/
Other
642 stars 57 forks source link

Better TypeScript types #41

Closed bengry closed 3 years ago

bengry commented 4 years ago

Add better support for TypeScript types:

  1. Add string literal for presets.
  2. Fix the type of IWantHueOptions.

This should be included as a patch version ideally, or a breaking one - if you consider invalid types that were allowed before past compile-time (which I don't). e.g. this works in v1.4.0:

import iwanthue from 'iwanthue';

const colors = iwanthue(2, {
  // This is obviously incorrect at runtime, but passes compile-time
  colorSpace: 'invalid',

  // All of these are required by type definitions today
  clustering: 'k-means',
  distance: 'cmc',
  colorFilter: () => true,
  quality: 1,
  seed: null,
  ultraPrecision: false
})

Following this change you'd get an error on line 5 (colorSpace: 'invalid') and would be able to omit the rest of the options.

bengry commented 3 years ago

@Yomguithereal seems like you added some minor TypeScript changes yesterday that this PR also addresses. I'll rebase it on top of the latest master in a bit, but it also addresses some other type-safety related issues, which I'd love to address, if you're already releasing new versions of this package (the last of which, before today, was 9 months ago).

So a friendly ping about this PR basically :). Thanks!

Yomguithereal commented 3 years ago

Hello @bengry. Sorry I completely missed your PR, I don't know how this got over my head. I indeed changed the types so that you can pass partial options to the function. Do you want to edit your PR to only include the part about presets so I can merge it and release a fix on npm :) ?

bengry commented 3 years ago

Hello @bengry. Sorry I completely missed your PR, I don't know how this got over my head. I indeed changed the types so that you can pass partial options to the function. Do you want to edit your PR to only include the part about presets so I can merge it and release a fix on npm :) ?

Done. Note that I also changed types to interfaces, since those allow type augmentation by users (9a42f8c).

Yomguithereal commented 3 years ago

I am having a hard time distinguishing between type and interface for plain objects in TS but anyway, let me merge and release your PR now :)

Yomguithereal commented 3 years ago

v1.4.2 is now live on npm

bengry commented 3 years ago

I am having a hard time distinguishing between type and interface for plain objects in TS but anyway, let me merge and release your PR now :)

They're mostly equivalent at this point, though each one has their own specific use-cases, especially when you get to more advanced types, for the purposes of this package - they're the same, except for the fact that interfaces allow for augmentation by any 3rd party types, as I mentioned above. See this example. In the more real-world use-case, the DogInterface with just the name is what this package, exposes, and I want to add age to its types. This doesn't allow you to override properties, but it's still useful at times, and types give no additional benefit in this use-case.

v1.4.2 is now live on npm

Thanks! Much appreciated. I can now delete the patch we had on our end (using patch-package).