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

When removing fields, rename new ones #39

Merged
merged 1 commit into from
Nov 20, 2019

Conversation

perrin4869
Copy link
Contributor

If the array of fields have different shapes, removing a field will result in new fields with corrupt state. According to

// when that field gets registered, with its own change/blur/focus callbacks
, the new field will get registered, and the correct functions will be added, but unfortunately, final form never does, since the following condition will fail: https://github.com/final-form/final-form/blob/4476c4539c9a9514a1ecbfdade974bd0aafd3b29/src/FinalForm.js#L775

I tried asserting that those 3 functions always exist, by adding the following lines to final-form's registerField:

// in case a mutator like final-form-arrays moves a field state without registering
state.fields[name].blur = state.fields[name].blur || (() => api.blur(name))
state.fields[name].change = state.fields[name].change || (value => api.change(name, value))
state.fields[name].focus = state.fields[name].focus || (() => api.focus(name))

And this fixed the issue too. But I thought this approach is a little bit better

@codecov
Copy link

codecov bot commented Aug 30, 2019

Codecov Report

Merging #39 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #39   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          12     12           
  Lines         139    141    +2     
  Branches       29     30    +1     
=====================================
+ Hits          139    141    +2
Impacted Files Coverage Δ
src/remove.js 100% <100%> (ø) ⬆️

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 7b37726...572f5fe. Read the comment docs.

@perrin4869
Copy link
Contributor Author

I think that maybe, this change should be moved inside of moveFieldState, and remove the possibility of a null change/blur/focus state altogether. But that is a slightly bigger change and breaks more tests

@erikras erikras merged commit 7ee1ef8 into final-form:master Nov 20, 2019
@erikras
Copy link
Member

erikras commented Nov 20, 2019

Published in v3.0.2.

huan086 added a commit to huan086/final-form-arrays that referenced this pull request Nov 11, 2020
Revert changes from final-form#39 that moves `fieldSubscribers`, causing the wrong field to be notified.
huan086 added a commit to huan086/final-form-arrays that referenced this pull request Nov 12, 2020
Revert changes from final-form#39 that moves fieldSubscribers, causing the wrong field to be notified.
huan086 added a commit to huan086/final-form-arrays that referenced this pull request Nov 12, 2020
Revert changes from final-form#39 that moves `fieldSubscribers`, causing the wrong field to be notified.
huan086 added a commit to huan086/final-form-arrays that referenced this pull request Nov 12, 2020
Revert changes from final-form#39 that moves `fieldSubscribers`, causing the wrong field to be notified.
huan086 added a commit to huan086/final-form-arrays that referenced this pull request Nov 13, 2020
Revert changes from final-form#39 that moves `fieldSubscribers`, causing the wrong field to be notified.
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.

2 participants