🐞 fix #11842 radio buttons not disabled when multiple share a name

[cj-young]

To fix #11842

I extended the functionality of iterateFieldsByAction to be able to handle multiple refs, which allows _disableForm to properly disable all inputs. Previously, when useForm is passed the disabled option, it only ever disables the first input of some name if multiple inputs share a name.

I don't know if this is the cleanest solution, but I didn't want to disrupt the structure of the current iterateFieldsByAction function.

[codesandbox]

Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders

Open Preview

[bluebill1049]

oh nice! thanks @cj-young i will take a look at this weekend!

[bluebill1049]

Does this works?

import { FieldRefs, InternalFieldName, Ref } from '../types';
import { get } from '../utils';
import isObject from '../utils/isObject';

const iterateFieldsByAction = (
  fields: FieldRefs,
  action: (ref: Ref, name: string) => 1 | undefined | void,
  fieldsNames?: Set<InternalFieldName> | InternalFieldName[] | 0,
  abortEarly?: boolean,
  iterateRefs?: boolean,
) => {
  for (const key of fieldsNames || Object.keys(fields)) {
    const field = get(fields, key);

    if (field) {
      const { _f, ...currentField } = field;

      if (_f) {
        if (iterateRefs && _f.refs && !abortEarly) {
          for (const ref of _f.refs) {
            action(ref, key)
          }
        } else if (_f.refs && _f.refs[0] && action(_f.refs[0], key) && !abortEarly) {
          break;
        } else if (_f.ref && action(_f.ref, _f.name) && !abortEarly) {
          break;
        } else {
          iterateFieldsByAction(currentField, action);
        }
      } else if (isObject(currentField)) {
        iterateFieldsByAction(currentField, action);
      }
    }
  }
};

export default iterateFieldsByAction;

[cj-young]

It seems like when the return value of the action is 1, the function breaks out of the loop and doesn't apply it to any other fields. I think either the naming or usage of abortEarly is backwards, since when it's false or undefined (the default), the function will abort when action returns 1, but if it's true, it won't abort early.

When applied to multiple refs, I guess it could either break out either after all the refs have been iterated over or after the first call returns 1.

If it immediately returns when the first call returns 1, this could work:

import { FieldRefs, InternalFieldName, Ref } from '../types';
import { get } from '../utils';
import isObject from '../utils/isObject';

const iterateFieldsByAction = (
  fields: FieldRefs,
  action: (ref: Ref, name: string) => 1 | undefined | void,
  fieldsNames?: Set<InternalFieldName> | InternalFieldName[] | 0,
  abortEarly: boolean = true,
  iterateRefs?: boolean,
) => {
  for (const key of fieldsNames || Object.keys(fields)) {
    const field = get(fields, key);

    if (field) {
      const { _f, ...currentField } = field;

      if (_f) {
        if (iterateRefs && _f.refs) {
          let shouldAbort = false;
          for (const ref of _f.refs) {
            shouldAbort = !!action(ref, key) && abortEarly;
            if (shouldAbort) break;
          }
          if (shouldAbort) break;
        } else if (_f.refs && _f.refs[0] && action(_f.refs[0], key) && abortEarly) {
          break;
        } else if (_f.ref && action(_f.ref, _f.name) && abortEarly) {
          break;
        } else {
          iterateFieldsByAction(currentField, action);
        }
      } else if (isObject(currentField)) {
        iterateFieldsByAction(currentField, action);
      }
    }
  }
};

export default iterateFieldsByAction;

I could replace the two break statements with a return, but the rest of the if blocks use breaks so I just continued doing that.

If it instead aborted after finishing iteration over the current field, the if block could be replaced with:

if (iterateRefs && _f.refs) {
  let shouldAbort = false;
  for (const ref of _f.refs) {
    shouldAbort = (!!action(ref, key) && abortEarly) || shouldAbort;
  }
  if (shouldAbort) break;
}

Does this look good? If so, which one is preferred?

[bluebill1049]

your second approach looks good, I am just trying to save some bytes here and simplify the code. good stuff 👍

[bluebill1049]

Thanks for the contribution @cj-young appreciated here.