final-form-arrays icon indicating copy to clipboard operation
final-form-arrays copied to clipboard

unshift and insert mutator is broken in v3.0.2

Open doytch opened this issue 4 years ago • 10 comments

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:

  • Append a customer. Fill out the name.
  • Prepend a customer.
    • Oops! The first and last name shown are those of the first, appended customer.
  • Change the prepended customer's name.
    • Oops! It don't work! Changes aren't reflect in the <input>!

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 avatar Nov 22 '19 16:11 doytch

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.

doytch avatar Nov 26 '19 15:11 doytch

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 avatar Jan 13 '20 21:01 petrbela

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.

petrbela avatar Jan 14 '20 02:01 petrbela

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

nik-lampe avatar Jun 19 '20 07:06 nik-lampe

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

Vishwas-93 avatar Feb 20 '21 01:02 Vishwas-93

Hello 👋 Any update? Thanks :)

vdesdoigts avatar Jun 03 '21 07:06 vdesdoigts

The issue still exists. The following sandbox works in [email protected], 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 avatar Aug 04 '21 17:08 naazim

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

naazim avatar Aug 04 '21 17:08 naazim

Any update on the fix?

navinleon avatar Mar 27 '22 08:03 navinleon

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

Elecweb avatar Jun 18 '23 18:06 Elecweb