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

Fixes unshift and insert row not update field's state correctly #96

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Elecweb
Copy link

@Elecweb Elecweb commented Jun 18, 2023

as reported in #44 and final-form/react-final-form-arrays#138.

This applied both unshift and insert because unshift use insert as this

const unshift: Mutator<any> = (
[name, value]: any[],
state: MutableState<any>,
tools: Tools<any>
) => insert([name, 0, value], state, tools)

I've investigated and have assumed the issue is in the following.

const pattern = new RegExp(`^${escapeRegexTokens(name)}\\[(\\d+)\\](.*)`)
const newFields = {}
Object.keys(state.fields).forEach(key => {
const tokens = pattern.exec(key)
if (tokens) {
const fieldIndex = Number(tokens[1])
if (fieldIndex >= index) {
// Shift all higher indices up
const incrementedKey = `${name}[${fieldIndex + 1}]${tokens[2]}`
copyField(state.fields, key, newFields, incrementedKey)
return
}
}
// Keep this field that does not match the name,
// or has index smaller than what is being inserted
newFields[key] = state.fields[key]
})

the logic is trying to shift the field's state to one for all fields that are greater than or equal to inserted index.
For the field's state at inserted index, it will leave as empty because there's a return statement here.

const incrementedKey = `${name}[${fieldIndex + 1}]${tokens[2]}`
copyField(state.fields, key, newFields, incrementedKey)
return
}
}
// Keep this field that does not match the name,
// or has index smaller than what is being inserted
newFields[key] = state.fields[key]
})
state.fields = newFields

For example, if there're 4 field array items and we used insert(2, value), the field's state will shifted like this

newState[0] = currentState[0]
newState[1] = currentState[1]
// newState[2] will be empty
newState[3] = currentState[2]
newState[4] = currentState[3]

this behavior, as I understand, it's intended and should be correct. because when the state is empty, it'll get the default state from here

https://github.com/final-form/final-form/blob/d20c44be8766b93424e7754fe2629820fd93d21a/src/FinalForm.js#L850-L866

which should set the default for the missing state. but the problem is related to React component's key prop

Since the component is rendered by each item in fields, as a convention and it should be, provided with name from fields. so the field component is reused with the same component as the previous, one before insert(2, value) is called and after because name are the same. So method registerField which should set the field's state as default, doesn't trigger. this makes the field's state incorrect and leads to rendering problems.

I've tried to make the key unique and found that it render correctly. but as we know, this is not a good solution.

Screen.Recording.2566-06-19.at.00.30.43.mov

Here are the possible solutions I can think of

  1. name should not be used as a key and the final form array provides another variable for this purpose instead.

Since the problem is about the rendering of components, we might need to provide a key which is different before and after insert. But I can't think of how we should generate a key that is new every time we insert a new index but also it should be generated based on an index. This is quite a conflict. so I have no idea about this. anyways, this should be done in https://github.com/final-form/final-form-arrays and shouldn't relate here.

  1. Just remove the shifting state and keep only copy value
    as suggested but this Insert does not cause array children to refresh correctly. react-final-form-arrays#138 (comment) which I still do not understand why it works as the field's state at inserted index just missing. Anyways, It might be some edge cases that lead to errors.

  2. Add default state for the inserted field.
    IMO, this might be the safest solution, which try to add default as intended. Fortunately, in the mutator, there's a helper function resetFieldState for resetting the field's state.

My PR is to use solution 3, as far as I have checked, it has no problem. I checked with validation for the array, shows an error at submit (this is required the use of the field's state).

(the validation is, the name field must have string "t")
https://github.com/final-form/final-form-arrays/assets/13183413/5c055b9e-9046-49f9-8974-d98a99df5954

For anyone, who can't wait for the fix, and need a quick solution now (I can feel that), I provided the gist you can copy and use the functions as the mutator.

https://gist.github.com/Elecweb/7f3478b01126e40ce98492de32d3f995

I hope this can help anyone get some insight into the issue and solutions 🙏

@Elecweb Elecweb changed the title fix: unshift and insert row not update useField correctly Fixes unshift and insert row not update field's state correctly Jun 18, 2023
@makhnatkin
Copy link

I had similar problems.

After many experiments, I came to the conclusion that I should abandon using an array in favor of normalized data - an array of IDs and a data object. This also allowed me to solve the issues with the unshift and move mutators.
https://codesandbox.io/s/react-final-form-rows-field-array-alternative-react-dnd-as-drag-drop-6vrpy6
https://github.com/makhnatkin/react-final-form-rows/blob/main/src/useRows.ts

it also helped me solve performance problems.
final-form/react-final-form#336 (comment)

@minh-le-jh
Copy link

@erikras Hello, could you have a look at this?

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

Successfully merging this pull request may close these issues.

3 participants