final-form / final-form-arrays

Array Mutators for 🏁 Final Form
https://final-form.org/arrays
MIT License
70 stars 40 forks source link

unshift and insert mutator is broken in v3.0.2 #44

Open doytch opened 4 years ago

doytch commented 4 years ago

Are you submitting a bug report or a feature request?

Bug Report!

What is the current behavior?

The unshift mutator doesn't work. When a field is unshifted onto an array, it's value is inited as null and any changes to field (eg, via <input>s) aren't reflected.

Sandbox Link

This is actually visible via a slight tweak to the standard RFF Arrays example. Rather than just an "Add customer" button, there's now Append and Prepend variants. I've also bumped versions on all FF libs to latest.

To reproduce:

What is the expected behavior?

The behaviour that you see if you go into the sandbox and change the FFA library version to 3.0.1

What's your environment?

FF 4.18.6 FFA 3.0.2 RFF 6.3.3 RFFA 3.1.1

doytch commented 4 years ago

Further debugging shows that .insert appears to be broken as well. A quick tweak of that CodeSandbox to use .insert(..., 0, ...) instead also fails. This is obviously expected, given that unshift uses insert in its implementation.

petrbela commented 4 years ago

I'm seeing the same issue, except in my case, downgrading to 3.0.1 doesn't help. https://codesandbox.io/s/react-final-forms-re-rendering-wup5c (you can ignore the "Selected", it's for testing state, which appears to be correct)

petrbela commented 4 years ago

Actually, your sandbox has a flaw in that it uses name as the key, which then results in an incorrect React tree (though a case could be made that RFFA should still be able to handle this). When I introduce id as the key (updated sandbox) then it results in the same error as I'm getting in my other sandbox:

Uncaught TypeError: state.change is not a function
    at eval (VM1130 react-final-form.cjs.js:587)
    at HTMLUnknownElement.callCallback (VM1104 react-dom.development.js:149)
    at Object.invokeGuardedCallbackDev (VM1104 react-dom.development.js:199)
    at invokeGuardedCallback (VM1104 react-dom.development.js:256)
    at invokeGuardedCallbackAndCatchFirstError (VM1104 react-dom.development.js:270)
    at executeDispatch (VM1104 react-dom.development.js:561)
    at executeDispatchesInOrder (VM1104 react-dom.development.js:583)
    at executeDispatchesAndRelease (VM1104 react-dom.development.js:680)
    at executeDispatchesAndReleaseTopLevel (VM1104 react-dom.development.js:688)
    at Array.forEach (<anonymous>)
    at forEachAccumulated (VM1104 react-dom.development.js:660)
    at runEventsInBatch (VM1104 react-dom.development.js:816)
    at runExtractedEventsInBatch (VM1104 react-dom.development.js:824)
    at handleTopLevel (VM1104 react-dom.development.js:4826)
    at batchedUpdates$1 (VM1104 react-dom.development.js:20439)
    at batchedUpdates (VM1104 react-dom.development.js:2151)
    at dispatchEvent (VM1104 react-dom.development.js:4905)
    at eval (VM1104 react-dom.development.js:20490)
    at Object.unstable_runWithPriority (VM1111 scheduler.development.js:255)
    at interactiveUpdates$1 (VM1104 react-dom.development.js:20489)
    at interactiveUpdates (VM1104 react-dom.development.js:2170)
    at dispatchInteractiveEvent (VM1104 react-dom.development.js:4882)

The slight difference in reproduction is that after first appending and then prepending a row, the culprit becomes the second row (which was moved from index 0 to 1), while the first row is mapped correctly.

Btw this bug seems to have existed since 3.0.1, while 3.0.0 results in updating the first row when you try to change the second :)

Whoever solves this logic should get a medal.

nik-lampe commented 4 years ago

It looks to me like the call to "moveFieldState" in the insert mutator might be the issue.

https://github.com/final-form/final-form-arrays/pull/35

This is used to move the state to the shifted fields, but it actually also deletes the field by moving. I think it should copy the state and then assign a "fresh" state to the field just added.

Or maybe first shift the states and then add the new value to the desired index?

But to be fair, I don't quite understand what the moveFieldState function actually does. I do understand that it copies the change, blur and focus functions and updates the name. But I'm not quite sure WHY it does this.

btw. remove function is broken, too

Vishwas-93 commented 3 years ago

Agreed, the notion of cloning the array [...array] might be incorrect because the array contains functions. I tried to fork the final-form-arrays and used lodash deep clone. It still doesn't fix the problem. I had very little time and hence, ended up making a new unshift(insert to the start of the array) that uses push logic but reverses the array before and after pushing a new element into the array.

Here's a copy of the code (Just for Unshift). I will look and try to fix the actual issue.

https://github.com/Vishwas-93/final-form-arrays/blob/master/src/unshift2.js

Thanks

vdesdoigts commented 3 years ago

Hello 👋 Any update? Thanks :)

naazim commented 3 years ago

The issue still exists. The following sandbox works in final-form-arrays@1.1.2, and breaks for any version above this: (Currently works, please bump the version from the left menu to see this failing) https://codesandbox.io/s/react-final-form-field-arrays-forked-g4tum?file=/index.js

Or if I'm doing something wrong, please let me know

naazim commented 3 years ago

Also I see that there's a forked fix that works. Any specific reason this fix is not merged? (We cannot use forked packages because of company security policies unfortunately) https://github.com/final-form/final-form-arrays/issues/64#issuecomment-841849808

navinleon commented 2 years ago

Any update on the fix?

Elecweb commented 1 year ago

I have an update about this issue and submitted the PR https://github.com/final-form/final-form-arrays/pull/96