Fix and optimize all mutators

[huan086]

This pull request depends on final-form/final-form#393

Fixes #58
Closes #43
Closes #45

I have edited the unit tests to trigger bugs, or to remove testing of implementation details.

Potentially breaking change

Change remove and removeBatch behavior to set array value to undefined when all items have been removed

I think this change would make pristine calculation more correct

[codecov]

Codecov Report

Merging #61 (59e9137) into master (4a65647) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master       #61   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        13    -2     
  Lines          165       175   +10     
  Branches        36        37    +1     
=========================================
+ Hits           165       175   +10     

Impacted Files	Coverage Δ
src/copyField.js	100.00% <100.00%> (ø)
src/insert.js	100.00% <100.00%> (ø)
src/move.js	100.00% <100.00%> (ø)
src/pop.js	100.00% <100.00%> (ø)
src/remove.js	100.00% <100.00%> (ø)
src/removeBatch.js	100.00% <100.00%> (ø)
src/swap.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a65647...71bebc7. Read the comment docs.

[huan086]

Don't merge yet. I realize I can simplify the logic and get rid of the sorting on fieldIndex

[souljja]

I hope that it'll be merged soon.

[souljja]

@erikras Could you tell if you gonna review/merge it soon? Because right now shift and insert methods are broken and I really need them in my application.

[jspizziri]

@erikras pinging you on this. There seem to be some pretty fundamental issues that need to be addressed in this lib.

Thanks for all the work!

[heikki]

This change broke our app because suddenly field array value was undefined instead of empty array when last item was removed. I don't understand the logic.

[gertdreyer]

The main bug fix in this PR fixed all the mutators except push that were broken and basically unusable.

The case @huan086 made was quite valid that the arrays behave differently when you clear them all other fields default to undefined in the empty state and this leads to pristine being wrong when your do not define initial values and then add and remove all elements from the array. Note that the new release was a minor change and not just a patch which is guaranteed to be compatible.

To get the behaviour as it was previously you can have a look at an identity function for the parse callback.

[peruukki]

this leads to pristine being wrong when your do not define initial values and then add and remove all elements from the array

But if you have defined an initial value of [], shouldn't it go back to it if you add and remove all items? Now it always goes to undefined.

[gertdreyer]

The strings in FF have been working with this approach.

If you define an empty string as an initial value and then add something to the field and remove it you end up with an undefined value after the change. The behaviour you want is not implemented for strings either (why should arrays be treated differently?) The change is quite simple and is documented in how to accomplish the alternative you need at https://final-form.org/docs/react-final-form/types/FieldProps#parse

Compare the sequence below in the codesandbox in the firstName field and lastName field.
firstName is the default behaviour, while lastName gets the behaviour you need.

https://codesandbox.io/s/zealous-julien-4qlo0l?file=/index.js

The initial value for both: ""
Enter data in the fields, and clear the data from the fields.
For firstName it is undefined.
For lastName it is "".

[peruukki]

Thanks for the info! I agree arrays should work the same way as strings, though I guess that just means I would also like strings to work differently. 😅 (Though not expecting that.)

Thanks for pointing out parse. 👍 It bothers me a bit that it can be easy to forget to add it, especially since an identity function looks like it does nothing, so it has surprising side effects. I know it's documented but it's definitely a gotcha for people who are not that familiar with the library.

Maybe there could be an option in e.g. FormProps to specify what kinds of default values to use, similarly to keepDirtyOnReinitialize? I understand you will want to keep the interface as small as possible, but just a suggestion or food for thought. 🙂 Ideally, that choice would propagate to the type definition of the field's value too, but not sure if that's even possible.

[peruukki]

The behaviour you want is not implemented for strings either (why should arrays be treated differently?)

I agree arrays should work the same way as strings [...]

Actually, it came to my mind that arrays are different — we even have this separate package for dealing with them. I believe in the most common use case, the form value contains values for a collection of inputs, not for a single input. So it's an added layer that doesn't correspond to a native input, and therefore I think a different behavior would be justifiable.

[huan086]

Hi @peruukki, as the author of this pull request, I don't like the current behaviour actually. The decision to make values undefined dates back to redux-forms, and I think it was designed this way so that pristine state gets computed correctly, where initial state is set to undefined and an object gets added and removed, thus setting it back to undefined. undefined === undefined, thus form state is pristine, while in the alternative [] === [] is false and would cause a false positive that the form has changes.

Ideally, the initial value should be consulted, so that pristine calculation is even more correct. E.g. Initial [] after adding and remove should go back to the same reference in the initial value. Or even a step further, if initial state is [1234] and items are added and removed, and ends up with [1234] again, the final [1234] should be the same reference as the initial value.

I am not sure how to implement the ideal behaviour while keeping the performance of the code. The current react-final-forms already performs poorly compared to redux-forms where I keyed array items with an id.

Please contribute to make this library better when you have the time, thanks!