HasManyFields mixes up values of uncontrolled inputs in rows

[tyleralves]

I believe the cause is using indices as keys for rows combined with using uncontrolled inputs. Since we remap the HMF value after each change, the HMFRow keys will always be 0 to {value.length - 1}. When react reconciles, it deletes the last row because its key no longer exists. With controlled inputs the last row is still deleted, but this isn't a problem because the values are also reassigned.

STR:

1. Pass template that contains uncontrolled inputs to HasManyFields
2. Delete any row besides the last row

O:

• Any uncontrolled input in the deleted row retains its value
• Uncontrolled inputs in the last row are deleted
• Any controlled input values are reassigned to proper row

D: Uncontrolled inputs should properly stay with the Rows they were created on

Here's a PR with a story that reproduces the bug:
#750

[tyleralves]

The easy workaround is to just use HMFRows (manually add unique key to each row) and HMFAdd so this probably isn't very time sensitive. I've added it to my FFF list, so plan on looking more into if no one gets to it sooner.

// Workaround; inline jsx mapping for ease of reading
{lineItems.map((item, index) => (
  <HasManyFieldsRow
    deletable={isDeleteShowing}
    key={item.id}
    onDelete={() => purchaseOrderStore.removeLineItem(index)}
  >
    <LineItemFormRow lineItemStore={item} />
  </HasManyFieldsRow>
))}
<HasManyFieldsAdd onClick={purchaseOrderStore.addLineItem}>
  Add Another Line
</HasManyFieldsAdd>

[darreneng]

uh so I'm pretty sure I caused this bug 😅
#535

I think most of the time developers should pass an array of objects that each have a key property` as a prop to this component. We might want to add that to the PropTypes (or TS interface) for this component. Something like:

PropTypes.arrayOf(PropTypes.shape({ key: PropTypes.string })

If not since we already have lodash.uniqueid in the package.json we can use that to automatically provide a key.

[tyleralves]

I guess, but It would be just as bad to regenerate the keys with every render. Instead of the wrong uncontrolled input being unmounted (this bug), all uncontrolled inputs would remounted and lose their values(pre #535)!

I agree with your type suggestion, but it is a bit more complicated since the HasManyFields itself can be uncontrolled (currently doesn't require a value). Issue #519 points out that no one currently uses it as an uncontrolled component and suggests removing its uncontrolled capability.

So maybe a future fix could be to remove HMF uncontrolled capability, and enforce the value type.