Run & review this pull request in StackBlitz Codeflow.

Walkthrough

The changes refactor option normalization in BFormRadioGroup to explicitly include only id, value, disabled, and text fields instead of spreading all original option properties. Index-based id generation is added for uniqueness. A new test validates label-to-input linkage with custom field names.

Changes

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Consider whether explicitly excluding unmapped option properties could break any undocumented usage patterns or component extensions.
• Verify that the index-based id generation strategy remains unique and consistent across re-renders and dynamic option updates.
• Ensure the test comprehensively covers both object options with custom field names and primitive options.

Suggested reviewers

• VividLemon

Poem

🐰 The options now whisper in structured refrains,
No stray attributes causing pains!
With id, value, disabled, and text held tight,
Each radio choice aligns just right!
Clean normalization makes children smile,
Form radio hops with newfound style! ✨

Pre-merge checks and finishing touches

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

What I've noticed is that because Id is uppercased in your example, it ends up getting processed as an attribute, rather than a prop. The handler in BFormCheckbox runs correctly, it appears to be correct in that when you spread the attributes, Id becomes an attribute id on the element. It appears correct, as that would be the expected output of the computedId, however, the truth of that is that it is due to it being processed as an attribute.

Then that also explains why computedId is wrong for the label. It doesn't see the uppercased Id as a prop, as that is being processed as an attribute. So it's building what is the computedId

This is what's happening, the attribute is taking over:

It appears like the reproduction is more an issue with using Vue than it is with the library. As if you were using the component variation, one would see (with TS) that you'd have a type issue

Interesting, but why is there a spread in the first place ? The docs says only 'value', 'text' and 'disabled' are supported. Is the purpose of the spread to support every props of BFormRadio/BFormCheckbox ?

Interesting, but why is there a spread in the first place ? The docs says only 'value', 'text' and 'disabled' are supported. Is the purpose of the spread to support every props of BFormRadio/BFormCheckbox ?

Perhaps its best to add in the documentation. The purpose is that anything could be forwarded, attributes and props alike. It's just that those specific values are special

Then, maybe we should prevent the forwarding of the attributes declared in textField / valueField ? I don't see a case where one would expect/need the properties contained in textField / valueField to be forwarded.

Something like:

const normalizeOptions = computed<
  {
    text: string | undefined;
    // eslint-disable-next-line @typescript-eslint/no-explicit-any
    value: any;
    disabled?: boolean | undefined;
    [key: string]: unknown;
  }[]
>(() =>
  props.options.map((el) =>
    typeof el === 'string' || typeof el === 'number'
      ? {
          value: el,
          disabled: props.disabled,
          text: el.toString(),
        }
      : (() => {
          const clone: Record<string, unknown> = { ...el };

          delete clone[props.valueField as string];
          delete clone[props.textField as string];
          delete clone[props.disabledField as string];

          return {
            ...clone,
            value: (el as any)[props.valueField],
            disabled: el[props.disabledField] as boolean | undefined,
            text: el[props.textField] as string | undefined,
          };
        })()
  )
);