[Request] Support non-string select / multiselect `value`s about clack HOT 10 CLOSED

Opened #102 with all Option instances updated so we can move discussion there

from clack.

Thanks for looking at this so quickly! Looking at your PR answers my original question a bit - seems like the primitive type restriction is due to values being potentially stringified (and subsequently displayed) if there's no label:

On one hand I can get behind this, as non-primitives may not stringify cleanly. However, it does prevent valid use-cases of:

• value type implementing a desired, human-readable toString method
• label being provided, meaning value is never stringified -> shown anyways

The second case in particular feels like enough of a reason to remove the primitive restriction. In the label-less case, (imo) the judgement call should be up to consumers of this lib on whether or not the value.toString() implementation is sufficient. (afaik) String(option.value) will always return some sort of string, so there shouldn't be any runtime error concerns either.

That all being said, that's just my opinion! Totally understand if you'd like to keep things as-is - happy to maintain my own fork / patch in projects. Thanks again!

from clack.

value type implementing a desired, human-readable toString method

Why not explicitly provide that as label then?

IMO, more complex types could be handled in user land without much overhead. They ought to have a unique identifier to use as value.

Finding the parent object from the returned value is trivial.

from clack.

Why not explicitly provide that as label then?

That's fair - I'd be happy to provide my own label, though the types restrict me from providing any non-primitive value regardless. For example:

const today = new Date();
const tomorrow = new Date(today);
tomorrow.setDate(tomorrow.getDate() + 1);

await select({
  message: '',
  options: [
    {
      label: `Today (${today.toLocaleDateString()})`,
      value: today, // type error
    },
    {
      label: `Tomorrow (${tomrorow.toLocaleDateString()})`,
      value: tomorrow, // type error
    }
  ],
});

Even though this behaves just fine at runtime and the date values are never stringified, the types prevent this type of use case. Stringifying the value in options + decoding the final result just to satisfy TypeScript feels like unnecessary overhead + complexity.

I could see this going either way:

• remove value type restriction altogether, allowing for the above case + label-less cases (leaving control to consumers on whether value.toString() makes sense)
• only allow non-primitive values when a label is also provided (this would likely require some TypeScript magic to support both cases)

from clack.

That's fair - I'd be happy to provide my own label, though the types restrict me from providing any non-primitive value regardless.

Right, I see your point.

Even though this behaves just fine at runtime and the date values are never stringified, the types prevent this type of use case. Stringifying the value in options + decoding the final result just to satisfy TypeScript feels like unnecessary overhead + complexity.

Like you've pointed out yourself it's not to please TypeScript per se, but rather the underlying implementation (i.e. allowing value as a fallback in a sensical way).

For the example at hand, doing a .toISOString() and then new Date() doesn't seem like much overhead, nor being complex to me. There's no performance issues to talk about either in such small data sets.

With that being said, with regards to your suggested solutions, I'm not in favor of # 1, but see no reason not to allow # 2, if technically possible (which I believe it is). I'll explore when I find the time.

from clack.

@chrissantamaria your example above is supported in the latest release.

from clack.

Really appreciate you taking a look at this! It does look like non-primitive types are allowed as values (🎉), but the return type of select is unfortunately unknown. Here's a full example (available on stackblitz as well):

import { select, isCancel } from '@clack/prompts';

const run = async () => {
  const today = new Date();
  const tomorrow = new Date(today);
  tomorrow.setDate(tomorrow.getDate() + 1);

  const value = await select({
    message: '',
    options: [
      {
        label: `Today (${today.toLocaleDateString()})`,
        value: today,
      },
      {
        label: `Tomorrow (${tomorrow.toLocaleDateString()})`,
        value: tomorrow,
      },
    ],
  });
  if (isCancel(value)) {
    return;
  }

  console.log({ value }); // value inferred as unknown
};

run().catch((e) => {
  console.error(e);
  process.exit(1);
});

Haven't had time to dig into the internals yet, though I'm happy to do so!

from clack.

Sorry for not getting back to you. That's a real bummer... why, oh why, TypeScript? Can't you be a little decent and help out, if only just a little?

I took a quick look at it but wasn't able to sort it out then. I'll try to get back to it ASAP. Maybe infer could help us out here?

Not sure what's worse, if we should revert this until properly fixed or not.

Thoughts @natemoo-re?

from clack.

I might be missing something, but just by reducing select to use a single Value generic (removing Options) it seems like the problem goes away: chrissantamaria@d174313

I imagine there's likely some reason this generic existed, though typechecking for the build still passes. If this feels like a reasonable solution I'm happy to put up a PR!

from clack.

Seems reasonable to me! Always good with fresh eyes...

What say you @natemoo-re?

Note: there are more components using Option which would need to be changed as well, but you're probably aware.

from clack.