Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Indexes as keys break components behaviour #116

Open
myagoo opened this issue Oct 10, 2019 · 24 comments
Open

Indexes as keys break components behaviour #116

myagoo opened this issue Oct 10, 2019 · 24 comments

Comments

@myagoo
Copy link

myagoo commented Oct 10, 2019

Are you submitting a bug report or a feature request?

a bug report

What is the current behavior?

If i have an array of two fields, and remove index 0, then component at index 0 is updated with value of component at index 1 and component at index 1 is deleted.

What is the expected behavior?

If i have an array of two fields, and remove index 0, then component at index 0 is deleted.

Exemple

Sandbox Link

Any example of this lib has this issue
https://codesandbox.io/s/kx8qv67nk5

What's your environment?

Other information

Clearly the problem here is that indexes are used as keys.

Maybe final-form-array could create unique keys at push and keep track of them during other operations and provide this key in FieldArrayRenderProps.

We could not rely either on value ref for maintaining the index => key mapping.

@myagoo
Copy link
Author

myagoo commented Oct 11, 2019

I tried to keep track of keys on my side but I encountered another problem

Calling fields.remove(index) trigger 2 renders, a first where the deleted field is still present, a second with the field correctly deleted.

Which prevents me from doing something like

<ButtonDelete onClick={() => {
   fields.remove(index)
   keys.remove(index)
}}/>

since the following will still have the deleted value, but the associated will be deleted

https://codesandbox.io/s/react-final-form-field-arrays-mrl4s

@myagoo
Copy link
Author

myagoo commented Oct 11, 2019

It seems that queuing keys handling in a microtask allow the key handling to take place between the "bad" and the "good" render
Also, exposing an unique id here could do the trick (if it was exposed under the meta object for example) : https://github.com/final-form/final-form/blob/8a76f4d953d4815a6385207d53ec538cd0134ed8/src/FinalForm.js#L778

@momimomo
Copy link

I'm having issues from this morning too with the same things.
However, the examples you provided work for me.
https://codesandbox.io/s/react-final-form-field-arrays-mrl4s // works
https://codesandbox.io/s/kx8qv67nk5 // works
Also, I had issues with useFieldArray not being present again in new version, even after the export fix. Maybe it's something else that is leading to this behavior?

@myagoo
Copy link
Author

myagoo commented Oct 11, 2019

https://codesandbox.io/s/react-final-form-field-arrays-mrl4s works cause I queued key handling in a microtask, which is not something I want to do

https://codesandbox.io/s/kx8qv67nk5 looks like its working, but the issue is present, it's just that the inputs handle well if they are bein updated instead of being deleted

Also, I just discovered the data Field api in final-form which is not exposed in react-final-form. If it was, we could simply do something like

<Field data={{id: Math.random()}}>{({input, meta})}<div key={meta.data.id}"></div></Field>

(the data prop is read once on field register, so the id will be stable forever)

@myagoo
Copy link
Author

myagoo commented Oct 11, 2019

I think I'm on the wrong path, the id should somewhere in the final-form-arrays or react-final-form-arrays lib

@momimomo
Copy link

momimomo commented Oct 11, 2019

Could you explain how https://codesandbox.io/s/kx8qv67nk5 is broken?
(or rather, how to reproduce the bug you are describing)

@momimomo
Copy link

Also, isn't this related?
#114

@myagoo
Copy link
Author

myagoo commented Oct 11, 2019

Could you explain how https://codesandbox.io/s/kx8qv67nk5 is broken?
(or rather, how to reproduce the bug you are describing)

https://codesandbox.io/s/react-final-form-field-arrays-tc2tk

Open the codesandbox console, add 2 customer with different values, then delete the first => it should not trigger the useEffect but it does

@myagoo
Copy link
Author

myagoo commented Oct 11, 2019

Also, isn't this related?
#114

Does not look like it

@myagoo
Copy link
Author

myagoo commented Oct 11, 2019

The best workaround so far seems to handle keys manually in microtasks as seen in https://codesandbox.io/s/react-final-form-field-arrays-mrl4s
I also updated the exemple to better show the issue :

  • open the codesandbox console
  • create 2 customer with different names
  • delete the first
  • you will see that the second is deleted in the console

@myagoo
Copy link
Author

myagoo commented Oct 11, 2019

Calling fields.remove(index) trigger 2 renders, a first where the deleted field is still present, a second with the field correctly deleted.

@erikras Any idea why there is an incorrect render just after calling fields.remove(index) and any other array mutators ?

@myagoo
Copy link
Author

myagoo commented Oct 11, 2019

After investigation of those 2 renders, there seems to be a bug of synchronization of field states

https://codesandbox.io/s/react-final-form-field-arrays-3t4ol

step to reproduce :

  • open the codesandbox console
  • create a customer
  • delete it
  • look at the log after the "removing 0" one

You will see that the form state and the fields value are not in sync

If it was not for this bug, manually managing my keys would be possible

@momimomo
Copy link

momimomo commented Oct 11, 2019

I see now. I reverted to:

    "final-form": "^4.12.0",
    "final-form-arrays": "1.1.2",
    "react-final-form": "^4.1.0",
    "react-final-form-arrays": "^2.0.3"

Also, I used your solution with microtasks.
It seems rather stable after some extended manual testing.
I'm also setting new uuidv4 on each push to array and keeping it as a reference for other functionalities.

Thanks for the exhaustive explanations! For now I'm sticking to this, until I find out if it really isn't caused by:
final-form/final-form@7abee7c#diff-1fbdeac5e3837fb021ccc625fb2d8442
It looks like the re-render is not triggered because we're setting the field as undefined, so the condition will be false?

@myagoo
Copy link
Author

myagoo commented Oct 24, 2019

@erikras @momimomo @ I just wanted to note that I'm an idiot and that manually handling the keys (without using microtask) works fine. It's just that my codesandbox used old versions of the final-form ecosystem.

👍

It still bad that the examples uses name (or index, its the same thing here) as keys

@momimomo
Copy link

I noticed that even though I was using explicit versioning, the resolved dependencies in my package-lock also contained the same versions of final-form-arrays and react-final-form-arrays as you had in sandbox. Probably due to my lack of initial deeper investigation in package-lock or some other unknown issues.

I also faced issues with validation and was constantly reverting to older versions. Your solution still works for me as I haven't bumped version yet 👍

Thanks for the update! I'll try updating soon!

@myagoo
Copy link
Author

myagoo commented Nov 9, 2019

UPDATE : After implementing in real life the manual handling of keys for fields array, I can say it works, most of the time, but It would be much simpler, less error prone and edge cases would be covered if key management was built in react-final-form-arrays.

@slaivyn
Copy link

slaivyn commented Nov 14, 2019

My solution to also handle initial data :

  const permanentId = useRef(0);
  ...
  const initialValues = ...map(.... => ({
     ...
     key: `permanentKey${permanentId.current++}`,
  })
  ...
  return (
    <FieldArray ...>
      {({ fields }) => (
        ...
        {fields.map((name) => (
          <div key={(idx(values, (_) => _.elements[i]) || {}).key}>
            ...
          </div>
        }
      )}
    </FieldArray>
    <Button onClick={() => push('elements', { key: `permanentKey${permanentId.current++}` })}>+</Button>
  )

@jmackay-hippocmms
Copy link

The most fundamental use-case for this package is adding and removing data-rows. It's a little surprising that such core functionality isn't handled internally.

@momimomo
Copy link

After almost half a year, I have found a solution to this problem that allows for testing the initialValues that includes using useMemo for the initialValues and passing manually values instead of fields to method that also manually stores the keys and indexes for reference.

@myagoo
Copy link
Author

myagoo commented Apr 14, 2020

I'm curious about your solution, can you provide a gist ?
On my side, I found this https://react-hook-form.com/api#useFieldArray
This is, to my knowledge (but I must be wrong), the only react form library that handle this issue.
And the API is quite nice

@momimomo
Copy link

momimomo commented May 18, 2020

Hey, here are some crucial parts of my solution:

  let [keys, setKeys] = useState({ data: [] });
  let [counter, setCounter] = useState({ count: 0 });
  const defaultRanges = useMemo(
    () => [
      { type: 'full' },
    ],
    [dataSource]
  );
...
    <Form
      onSubmit={onSubmit}
      decorators={[focusOnError]}
      mutators={{
        ...arrayMutators,
      }}
      initialValues={{
        ranges: defaultRanges,
      }}
      render={({
        handleSubmit,
        form,
        form: {
          mutators: { push },
        },
        values,
        submitting,
 ...
                  <FieldArray name="ranges">
                    {({ fields }) => {
                      return (
                        <div>
                          {fields &&
                            fields.length > 0 &&
                           fields.map((name, index) => {
                              const range = fields.value[index] || {};
                              const { dates = {} } = range;
...

// in <Field /> important thing - use name as field name to have proper field id
...
    name={`${name}.dates`}
...

// in onClick on adding a new field
...
                              onClick={() => {
                                const id = uuid(); // using uuidv4
                                const lr = values.ranges[fields.length - 1]; // this will be up to date for other functions
                                const cb = () => {
                                  push('ranges', {
                                    dates: {},
                                    dayType: 'full'
                                  );
                                };
                                if (fields.length) {
                                  let newKeys = keys.data;
                                  let count = counter.count;
                                  newKeys.push({ count, id });
                                  setKeys({ data: newKeys });
                                  setCounter({ count: count + 1 });
                                }
                               ...other functions using current values
                              }}
...

// Delete button
...
                                          <Button
                                            type="button"
                                            role="button"
                                            onClick={() => {
                                              queueMicrotask(() => {
                                                let newKeys = keys.data;
                                                onRangeDelete(
                                                  range,
                                                  keys.data[index].id
                                                );
                                                newKeys.splice(index, 1);
                                                setKeys({
                                                  data: newKeys,
                                                });
                                              });

                                              fields.remove(index);
                                            }}
                                          >
                                            Delete
                                          </Button>
...

Unfortunately, my code includes much more utility methods and they contain a lot of business logic that is intersecting with the UI beahvior, but the basic solution should be visible here.

first package resolution that was working:

    "final-form": "^4.12.0",
    "final-form-arrays": "1.1.2",
    "react-final-form": "^4.1.0",
    "react-final-form-arrays": "^2.0.3",

My current working package versions from package-lock:

    "final-form": "4.18.6",
    "final-form-arrays":  "1.1.2",
    "react-final-form": "4.1.0",
    "react-final-form-arrays": "2.0.3",

@wangsen1989
Copy link

my solution is

import { useRef } from 'react';
import { useFieldArray, UseFieldArrayConfig, FieldArrayRenderProps } from 'react-final-form-arrays';

export type SimpleFields<FieldValue> = Omit<
  FieldArrayRenderProps<FieldValue, HTMLElement>['fields'],
  'forEach' | 'insert' | 'move' | 'update' | 'pop' | 'shift' | 'swap' | 'unshift'
>;

export function useFieldArrayWithUniqueKey<FieldValue>(
  name: string,
  config?: UseFieldArrayConfig<FieldValue> | undefined
) {
  const {
    fields,
    fields: { forEach, insert, move, update, pop, shift, swap, unshift, ...otherFieldsProp },
    ...other
  } = useFieldArray<FieldValue>(name, config);

  const keys = useRef<number[]>(new Array(fields.length).fill('').map(() => Math.random()));

  const push = (val: FieldValue) => {
    keys.current.push(Math.random());
    fields.push(val);
  };

  const remove = (index: number) => {
    keys.current.splice(index, 1);
    const val = fields.value[index];
    fields.remove(index);
    return val;
  };

  function map<R>(iterator: (name: string, index: number, key: number) => R) {
    return fields.map((name, index) => iterator(name, index, keys.current[index]));
  }

  return {
    ...other,
    fields: {
      ...(otherFieldsProp as SimpleFields<FieldValue>),
      push,
      remove,
      map,
    },
  };
}


import React, { useEffect } from 'react';
import { SimpleFields, useFieldArrayWithUniqueKey } from './useFieldArrayWithUniqueKey';

function List() {
  const { fields } = useFieldArrayWithUniqueKey(name, { initialValue: ref.current });

  return (
    <>
        <ul>
            {
               fields.map((name, index, key) => {
                   return (
                     <li  key={key}><MyInpt /> <Button onClick={() => fields.remove(index)}> remove </Button> </li>
                  )
                })
            }        
        </ul>
       <Button onClick={() => fields.push('')}> add </Button>
    </>
  );
}


@jaryway
Copy link

jaryway commented Dec 31, 2021

@erikras i have same problem!

it seem that, the officel demo everything is fine.
https://codesandbox.io/s/react-final-form-field-arrays-forked-bb6ly?file=/index.js
125

final-form-arrays@1.1.2 is fine.

but when i update version of libs, i got the same problem!
https://codesandbox.io/s/react-final-form-field-arrays-forked-rczfc?file=/index.js:2279-3037
124

sorry for my poor english!

look forward to your reply,thanks!

@MilllerTime
Copy link

Indexes as keys work great until each array element's component renders different sub-fields. Then all sorts of weird things start to happen when rearranging or deleting items, as described in this issue. I came to the conclusion that I needed final-form to maintain a unique ID for each array element for use as a React key, then found this issue and it really seems like this is the way to go.

I'd love to see FieldArrayRenderProps.map() provide a unique key tracked by final-form-arrays, in addition to name and index.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants