Closed danieldickison closed 2 years ago
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>
...
I reopened this because the compatibility issue is still relevant.
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.
@brianmhunt just added a test. It'll fail if you revert the change to selectExtension.ts.
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 🤞🏻
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
😁 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.
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 https://github.com/knockout/tko/pull/155#issuecomment-962090949, 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.
I prefer the more specific comparison.
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:I'll try to make a test for this if I have time.