Checkbox options should not mutate original object

[hisuwh]

Describe the bug Passing an array of options to formulate-input with a numeric value converts the value to a string. And much more worryingly actually mutates the options object on your component changing the values to a string (see codepen). It even does this if the type of your input does not use options (e.g. text).

To Reproduce Steps to reproduce the behavior:

1. Define an options object on your component with numbers for value:

   options: [
       { label: "First", value: 1 },
       { label: "Second", value: 2 },
       { label: "Third", value: 3 }
   ]
   

2. Pass this options object as a prop to any FormulateInput

   <FormulateInput :options="options" />
   

3. Observe the options in Vue dev tools

Reproduction https://codepen.io/hisuwh/pen/QWpqEme

Expected behavior I would expect vue-formulate to preserve the type of the value passed in (in my case I'm using a custom component which can handle numeric values even if vue-formulate internals cannot).

I would definitely not expect vue-formulate to mutate props it is passed in.

Device information: N/A

[hisuwh]

Seems the offending line of code is here: https://github.com/wearebraid/vue-formulate/blob/10ab31b4939323ed9d61cf810eddc676c4242bd1/src/libs/context.js#L414

Looks like you are explicitly not allowing numbers as values? Why is that? Also you mutate the option before you do object assign:

function createOption (option) {
  // Numbers are not allowed
  if (typeof option === 'number') {
    option = String(option)
  }
  if (typeof option === 'string') {
    return { label: option, value: option, id: `${this.elementAttributes.id}_${option}` }
  }
  if (typeof option.value === 'number') {
    option.value = String(option.value) // mutates the original option on the prop
  }
  return Object.assign({
    value: '',
    label: '',
    id: `${this.elementAttributes.id}_${option.value || option.label}`
  }, option)
}

This would fix the mutation whilst keeping the conversion of number to string (which I don't believe to be necessary)

function createOption (option) {
  // Numbers are not allowed
  if (typeof option === 'number') {
    option = String(option)
  }
  if (typeof option === 'string') {
    return { label: option, value: option, id: `${this.elementAttributes.id}_${option}` }
  }
  return Object.assign(
    {
      value: '',
      label: '',
      id: `${this.elementAttributes.id}_${option.value || option.label}`
    }, 
    option,
    {
      value: typeof option.value === 'number' ? String(option.value) : option.value
    })
}

You're only explicitly handling number though? So I could pass an array, or an object or any other type and they will get through.

[hisuwh]

In fact I've updated my codepen with objects as values and this works without issue. If you can support object values I don't see why you can't support numeric values.

[justin-schroeder]

@hisuwh the only reason for this is that HTML inputs don't support numeric values (strings only, cause...you know html). You can use non-string values in your custom components so we dont want to force-cast everything, but inputs like checkbox and selects don't allow numbers and it was a constant pain point for users. Vue Formulate used to not cast these to strings, but every week we got an issue about why numeric-keyed values didn't work so alas numbers are cast to strings.

[hisuwh]

You've closed this issue but you've not addressed the mutation point which definitely needs fixing.

Also html does not support numbers but virtual Dom does and it works fine to pass a number as a value to a select option in Vue from my experience. The extensibility of this library is great but mutating props passed through kind of breaks that.

Surely the responsibility of handling numbers vs strings should lie at the lowest level where they are actually rendered?

[justin-schroeder]

I agree that the mutation is wrong 😑 That will be fixed in version 3 which is a full rewrite, or sooner 👍 and I'll re-open to reflect that.

This library is opinionated by nature, so you might bump up against some things you disagree with (sorry!) In our experience maintaining the package it became clear that the string casting would benefit a significant percentage of users, but we have other plans for how to address this in version 3.

You are of course, welcome to use a different library if this one is too far off your expectations (I honestly mean that in a non-adversarial way).

But yes - agreed that the mutation needs to be fixed.

[hisuwh]

P.S. happy to send a PR for the mutation problem.

I guess the workaround if I really need numbers to use slotProps - it's just a messy and inconsistent, especially when I can pass through objects as values no problem.

[hisuwh]

You are of course, welcome to use a different library if this one is too far off your expectations

I guess the problem is this library is very close to my expectations. That is what is most frustrating - because I do really love what you are doing with it on the whole. Casting the strings is just somewhat unexpected, not documented and inconsistent when objects pass through unscathed (not suggesting you change that, please don't change that 😆). I do appreciate that there are probably other use cases that I have not encountered/considered and a library like this has to be semi-opinionated.

And there is a workaround - that just creates an inconsistent developer experience in my codebase.