survey icon indicating copy to clipboard operation
survey copied to clipboard

Ensure accurate selected index returned from Select

Open mislav opened this issue 3 years ago • 1 comments

Fixes https://github.com/AlecAivazis/survey/issues/412 which describes the case where the selected index would be mis-reported when some options happen to share identical value. Since the Select component internally tracks the selectedIndex, just use that value instead of trying to determine it by comparing strings.

@AlecAivazis In my attempts to clean up default value handling here, I fear I have introduced backwards-incompatible changes. I could restore the previous behavior, sure, but I wanted to ask if it's also worth "fixing" the default value handling. Namely:

  • Before, when a string value was passed, for example Default: "moo", this default was used in the result when the user immediately pressed Enter (i.e. without otherwise interacting with the Select component) even if the value moo did not exist among Options. Now, the prompt will error out noting that the default value must be present among Options, mainly so that it can be visually pre-selected when rendering the widget.

  • Before, when an invalid int value was passed, for example Default: 1000 when there are fewer than 1000 Options, a panic would happen when the user immediately pressed Enter, but not otherwise. Now, the prompt will immediately error out noting that the Default value is invalid since it's out of bounds.

So, what I changed here felt like fixing bugs to me, but I can imagine that some users might be relying on these features, for example having a Default that's not among Options so they can detect when the user wanted to skip a Select. I'm not sure if those features were worth keeping, though. For example, consider this prompt:

? Choose a country:  [Use arrows to move, type to filter]
> Afghanistan
  Åland Islands
  Albania

If I press Enter here, I would expect that "Afghanistan" got selected. However, if there was a Default value such as "moo", that string will be used instead of "Afghanistan", which I find counter-intuitive. I would rather break this feature than have users rely on it.

What are your thoughts?

mislav avatar May 12 '22 13:05 mislav

Oh, man. This is one of those super tough situations. I agree with you that these are really bug fixes but it seems very likely that someone out there is relying on the silent failure or doesn't know they have an invalid default value.

I'm leaning more towards keeping this under v2 and we should just make sure its very clear what happened in the release notes.

AlecAivazis avatar May 13 '22 18:05 AlecAivazis

@mislav i'm going to merge this since it's been around for so long and we both agree it's an appropriate change

AlecAivazis avatar Sep 12 '22 07:09 AlecAivazis

@AlecAivazis Thanks! Do you still think this should be a v2? Strictly speaking, I think it should.

mislav avatar Sep 12 '22 14:09 mislav