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

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

Open
doytch opened this issue Nov 22, 2019 · 10 comments
Open

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

doytch opened this issue Nov 22, 2019 · 10 comments

Comments

@doytch
Copy link

doytch commented Nov 22, 2019

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 doytch changed the title unshift mutator is broken unshift mutator is broken in v3.0.2 Nov 22, 2019
@doytch doytch changed the title unshift mutator is broken in v3.0.2 unshift and insert mutator is broken in v3.0.2 Nov 26, 2019
@doytch
Copy link
Author

doytch commented Nov 26, 2019

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
Copy link

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
Copy link

petrbela commented Jan 14, 2020

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
Copy link

nik-lampe commented Jun 19, 2020

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

#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
Copy link

Vishwas-93 commented Feb 20, 2021

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
Copy link

Hello 👋 Any update? Thanks :)

@naazim
Copy link

naazim commented Aug 4, 2021

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
Copy link

naazim commented Aug 4, 2021

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)
#64 (comment)

@navinleon
Copy link

Any update on the fix?

@Elecweb
Copy link

Elecweb commented Jun 18, 2023

I have an update about this issue and submitted the PR
#96

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

8 participants