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

Moving subfields of array field mix their values #692

Open
pedro757 opened this issue Apr 27, 2024 · 6 comments
Open

Moving subfields of array field mix their values #692

pedro757 opened this issue Apr 27, 2024 · 6 comments
Labels

Comments

@pedro757
Copy link

pedro757 commented Apr 27, 2024

Describe the bug

When the input is still idle (I mean undefined) and you try to change its position it takes the value of other subfields. It messes things up.

Codesandbox

https://codesandbox.io/p/sandbox/focused-pond-s96spf?file=%2Fsrc%2FApp.js%3A169%2C8

Steps to reproduce

  1. Try to move the empty subfields dragging and dropping in other position
  2. Now try to move subfields with non-empty values

Expected behavior

I expect it to keep its value.

@crutchcorn
Copy link
Member

I think this is a bug with our moveValue method. I'll investigate in a bit

@crutchcorn crutchcorn added the bug label Apr 29, 2024
@t-rosa
Copy link
Contributor

t-rosa commented Jun 22, 2024

I think this is a bug with our moveValue method. I'll investigate in a bit

If it saves a little time, after some research, I wrote a test to check whether there was a problem with the moveValue function, but the test went well.

I wonder if the problem isn't the collision between an uncontrolled and a controlled field, and that screws up the ui.

  it('should correctly move an array value, even with undefined values', () => {
    const form = new FormApi({
      defaultValues: {
        users: [
          {
            name: undefined,
          },
          {
            name: 'one',
          },
          { name: 'two' },
          {
            name: 'three',
          },
          {
            name: undefined,
          },
        ],
      },
    })

    const field = new FieldApi({
      form,
      name: 'users',
    })

    field.moveValue(3, 4)

    expect(field.getValue()).toStrictEqual([
      {
        name: undefined,
      },
      { name: 'one' },
      { name: 'two' },
      {
        name: undefined,
      },
      {
        name: 'three',
      },
    ])
  })

@crutchcorn
Copy link
Member

Hey @Balastrong, was this fixed when you updated your move and insert stuff?

@Balastrong
Copy link
Member

Balastrong commented Jun 23, 2024

Unfortunately I think there's still something not behaving here. I made a short example: https://stackblitz.com/edit/tanstack-form-grdmqr?file=src%2Findex.tsx&preset=node

First issue is that indeed having <input value={subField.state.value} ... /> can't work if value changes from string to undefined and vice-versa when moving values, but this may be on React and subField.state.value ?? '' can help.

The second issue is that if we use the move button and then we submit, the old value "one" remains in the input, but again this is probably still related to undefined being put there.

@Balastrong
Copy link
Member

Balastrong commented Jun 23, 2024

This test on react-form fails because magically the second inputs gets back the value 'one' on submit.
The same operation (move then submit) on form-core passes.

Failing Test

it('should correctly move an array value, even with undefined values and not alter the state after submit', async () => {
    function Comp() {
      const form = useForm({
        defaultValues: {
          people: [
            {
              name: undefined,
            },
            {
              name: 'one',
            },
          ],
        },
      })

      return (
        <div>
          <form
            onSubmit={(e) => {
              e.preventDefault()
              e.stopPropagation()
              form.handleSubmit()
            }}
          >
            <form.Field name="people" mode="array">
              {(field) => {
                return (
                  <div>
                    {field.state.value.map((_, i) => {
                      return (
                        <form.Field key={i} name={`people[${i}].name`}>
                          {(subField) => {
                            return (
                              <div>
                                <label>
                                  <div>Name for person {i}</div>
                                  <input
                                    value={subField.state.value ?? ''}
                                    onChange={(e) =>
                                      subField.handleChange(e.target.value)
                                    }
                                  />
                                </label>
                              </div>
                            )
                          }}
                        </form.Field>
                      )
                    })}
                    <button type="button" onClick={() => field.moveValue(1, 0)}>
                      Move 1 to 0
                    </button>
                  </div>
                )
              }}
            </form.Field>
            <form.Subscribe
              selector={(state) => [state.canSubmit, state.isSubmitting]}
              children={([canSubmit, isSubmitting]) => (
                <>
                  <button type="submit" disabled={!canSubmit}>
                    {isSubmitting ? '...' : 'Submit'}
                  </button>
                </>
              )}
            />
          </form>
        </div>
      )
    }

    const { getByText, findByLabelText, queryByText, findByText } = render(
      <Comp />,
    )

    const input = await findByLabelText('Name for person 0')
    expect(input).toBeInTheDocument()
    expect(input).toHaveValue('')

    const input2 = await findByLabelText('Name for person 1')
    expect(input2).toBeInTheDocument()
    expect(input2).toHaveValue('one')

    await user.click(getByText('Move 1 to 0'))

    expect(input).toHaveValue('one')
    expect(input2).toHaveValue('')

    await user.click(await findByText('Submit'))

    expect(input).toHaveValue('one')
    expect(input2).toHaveValue('') // This gets value 'one'
  })

@dlindahl
Copy link

dlindahl commented Dec 20, 2024

I have run into this similar, if not same problem. I have an array of objects that represent other sub-fields of various depths. The internals of moveFieldValues did not work, throwing an error when attempting to slice off the fromIndex value. IIRC, the error was attempting to delete fromIndex from object Array which was a very strange error. I got around that by using [arrayMove utility from @dnd-kit/sortable])(https://github.com/clauderic/dnd-kit/blob/master/packages/sortable/src/utilities/arrayMove.ts). My only guess as to why this works is that it is making a copy of the array before performing the splice operations?

Once that was worked around, I noticed that if one of the sub-fields that has an undefined value is repositioned with a sibling that does have a defined value, the sub-field will assume that siblings value.

If I do not render the FieldApi instance, then the value does not get "absorbed".
If I ensure that the value is an empty string rather than `undefined, then the value does not get "absorbed"

I think the problem is that while the internal form store state and field instance states reflect the desired overall state of the form, the FormApi#fieldInfo values do not get updated along with the store state so everything gets "merged" on the next render and the undefined value is now defined from its sibling. 🤷‍♂

EDIT: I'm on 0.34.1

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

No branches or pull requests

5 participants