Use == for comparing select option values for better ko compatibility

[danieldickison]

Knockout uses == for comparing select option values against the value binding value, and this PR restores that behavior for better compatibility. This is critical if you have bindings like this:

<select data-bind="value: typeID">
  <option value="1">foo</option>
  <option value="2">bar</option>
  ...

I'll try to make a test for this if I have time.

[mbest]

The problem with this change is that although typeID might start as a number, it will get updated to a string when the user selects something. Instead, set your options to have the numeric value:

<select data-bind="value: typeID">
  <option data-bind="value: 1">foo</option>
  <option data-bind="value: 2">bar</option>
  ...

[mbest]

I reopened this because the compatibility issue is still relevant.

[brianmhunt]

Thanks for the PR @danieldickison and for your thoughts @mbest !

I haven't got my head wrapped around this. Just to help me understand the problem, might this help:

    const strictEqual = optionValue === value
    const blankEqual = optionValue === '' && value === undefined
    const numericEqual = typeof value === 'number' && Number(optionValue) === value
    if (strictEqual || blankEqual || numericEqual) {

It'd really help me if we had a test that fails and illustrates the issue.

[danieldickison]

@brianmhunt just added a test. It'll fail if you revert the change to selectExtension.ts.

[brianmhunt]

Thanks @danieldickison -- what do you think of the more explicit equality test in my comment above? I'd like to avoid linters complaining about the "evil twins" and I think the more explicit we can be the easier to maintain 🤞🏻

[danieldickison]

Personally, I think just going with == along with a lint-suppression comment would be clearer than reimplementing that behavior explicitly, but that's a loosely-held opinion :)

Specifically, I think your solution should work fine, though maybe this bit:
const blankEqual = optionValue === '' && value === undefined
should be loosened up a bit to accept false/null/0 values:
const blankEqual = optionValue === '' && !value

[brianmhunt]

😁 Hmm... I think the clarity of the code is important, but more important is defining and testing the conditions.

Do we want 0 and false to be considered "blank"? I think undefined certainly, but null might also be a "positive" value.

@mbest any thoughts?

Not to drag the discussion out, but just want to be sure we end up with the expected behaviour.

[nmocruz]

That why this 4 version is taking so much time. People takes 2 months to decide something like this, and the decision is scale out to someone else.

…

On Friday, 5 November 2021, Brian M Hunt ***@***.***> wrote: 😁 Hmm... I think the clarity of the code is important, but more important is defining and testing the conditions. Do we want 0 and false to be considered "blank"? I think undefined certainly, but null might also be a "positive" value. @mbest <https://github.com/mbest> any thoughts? Not to drag the discussion out, but just want to be sure we end up with the expected behaviour. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#155 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHGRZJSXJYCYKZ6KNGQZWTUKQQR7ANCNFSM5DJ2HDPA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[mbest]

I prefer the more specific comparison.