[@lion/ui/localize.js] parseNumber('--42') is parsed as NaN

[riovir]

Expected behavior

Calls of parseNumber('--blah42blah') or parseNumber('--42') would only return with a type === 'number' that's a usable number. Based on parseNumber('++++blah42blah') and parseNumber('++++42') returning 42. I'd expect the same to work with multiple minus signs.

Actual Behavior

The parseNumber('--blah42blah') or parseNumber('--42') calls return with a NaN.

Additional context

Considering how NaN !== NaN this turned out to be the source of an infinite update loop that froze the browser tab if the customer accidentally tapped the '-' sign twice when entering a number.

Note that the loop is not directly caused by Lion, but is prone to happen when the LionInputAmount is used together with a LionForm in some apps.

[riovir]

I'm planning on posting a PR for this line: https://github.com/ing-bank/lion/blob/master/packages/ui/components/localize/src/number/parseNumber.js#L118

const cleanedInput = matchedInput.join(''); // Instead of this
const cleanedInput = dropExtraMinuses(matchedInput.join('')); 

Where dropExtraMinuses() keeps the first '-' and wipes the rest of the occurrences from the string. So '---42' -> '-42'

[tlouisse]

@riovir Thanks for proposing the fix!

Which component causes the infinite loop if I may ask? LionInputAmount?

[riovir]

Yes, coupled with the LionForm. Both wrapped as Vue components. The model-value-changed event is hooked up to result in a Vue event of update:modelValue. This in essence offers teams the means to opt-in to 2-way binding (https://vuejs.org/guide/components/v-model.html)

Here's a simplified template:

<!-- myFormData holds the value of { counter: 0 } it's a Vue Ref -->
<VueLionForm v-model="myFormData">
  <form>
    <VueLionInputAmount name="counter" label="Counter" />
  </form>
</VueLionForm>

In essence here's what happened:

• the customer entered something that resulted in a NaN on the input amount. (Like 999999999999999999999999999999999, or --42) From an a11y perspective, think twitchy finger or a dodgy low-profile keyboard
• the LionInputAmount dispatches model-value-changed with event.target.modelValue being NaN
• LionForm accepts the value dispatching its own model-value-changed
• the Vue wrapper pushes the value into the Ref of myFormData, then sets the modelValue on the LionForm
• it's a new object reference, so the form concludes the value has changed and tells its children to update
• the LionInputAmount sees the incoming modelValue with NaN and even though the old value is also NaN, since NaN !== NaN and typeof NaN === 'number' the component accepts the "new" value, starting the cycle again.

The customer sees a frozen tab. Naturally, this cycle can (and will) be stopped on multiple levels. That said, my change has primarily been motivated by mimicking what would happen if the '-' character were replaced with a '+'. The parseNumber() is pretty robust in making sense even from a 4+p0t4t0, but more than one '-' tripped it up.

[tlouisse]

Thanks for the good explanation 👍

Maybe we are also able to catch this here: https://github.com/ing-bank/lion/blob/master/packages/ui/components/form-core/src/FormatMixin.js#L81 (probably using the notEqual from lit here, would also fix it)

[riovir]

Verified the change and indeed it also fixes the infinite loop. I'm not fully sure how much one would like the parseNumber to make sense of something like '--42'. I'm leaning towards your proposed solution, and clising the submitted PR with the parseNumber tweak.

[riovir]

One question though: when the value of LionInputAmount is resulting in a NaN wouldn't a new Unparseable(this.value) work better? In our apps we type the modelValue as number | null | Unparseable. What 95% of app teams don't do is to write code that defends against NaN after they checked that the typeof modelValue === 'number'.

One possible approach here was my change to the parseNumber which is not enough, as we also got a reported bug about the LionInputAmount not handling a value like "999999999999999999999999999999934.42" well also resulting in a NaN. (In fact this was the original value that triggered the infinite loop: a customer pasting a waaay too big number)

[tlouisse]

One question though: when the value of LionInputAmount is resulting in a NaN wouldn't a new Unparseable(this.value) work better? In our apps we type the modelValue as number | null | Unparseable. What 95% of app teams don't do is to write code that defends against NaN after they checked that the typeof modelValue === 'number'.

One possible approach here was my change to the parseNumber which is not enough, as we also got a reported bug about the LionInputAmount not handling a value like "999999999999999999999999999999934.42" well also resulting in a NaN. (In fact this was the original value that triggered the infinite loop: a customer pasting a waaay too big number)

Really good point, Unparseable would be a way better fit (also for giving better feedback in the validation). Would you mind to do a PR for this? :) Or otherwise create an issue?

[riovir]

Created issue: https://github.com/ing-bank/lion/issues/2233 I either will be able to submit a PR this afternoon or won't be able for some time.

[tlouisse]

The best way to incorporate the hasChanged check (accommodating for NaN === NaN as well) in sync updates inside FormatMixin, would be wrapping up this wip PR: https://github.com/ing-bank/lion/pull/1914/files#diff-34fc2e999d85288efe2f26617206b27c1b6ccf878fb1464f02109b46c74d63b5L78