Skip to content

Fixed FieldArray re-render on every change when subscription is empty… #100

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

Merged
merged 1 commit into from
Aug 4, 2019

Conversation

jdimovska
Copy link
Contributor

Hey,

This PR fixes #93
I created a test where we pass an empty subscription object to the FieldArray and we check if only the Field whose value has changed gets re-rendered.
The test passes with this fix.

@codecov
Copy link

codecov bot commented Jul 18, 2019

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #100   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           6      6           
  Lines          57     60    +3     
  Branches       10     10           
=====================================
+ Hits           57     60    +3
Impacted Files Coverage Δ
src/useFieldArray.js 100% <ø> (ø) ⬆️
src/testUtils.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 24bdf4e...519a093. Read the comment docs.

@erikras
Copy link
Member

erikras commented Jul 18, 2019

Seems like this is going to necessarily break the value API... #96

Well, I guess if you provide a subscription prop and don't include value, then you don't get the value...?

@oliverlaz
Copy link

oliverlaz commented Jul 29, 2019

Hi @erikras, Hi @jdimovska,

I had a look at the linked issue and the provided fix.
With these changes, value subscription is disabled by default on empty object subscription.
Edit: On a second thought, Isn't this the expected behavior? Shouldn't subscription={{}} mean no-subscription?

Maybe an alternative approach would be to let the defaults (value, length) be overridden by the subscription prop on <FieldArray />

<FieldArray subscription={{ value: false }} />

In order this approach to work, this code should be changed from:
subscription: { ...subscription, value: true, length: true }, to
subscription: { value: true, length: true, ...subscription }.

With this change, all existing tests together with the new one pass successfully.
This change should be backwards compatible and shouldn't cause any side-effects if this library is used as recommended. However, it would be good if it is mentioned in the docs.

What are your thoughts on this?

@callmeberzerker
Copy link

Hi all,

subscription={{}} is definitely how ReactFinalForm interprets "no subscription" on the Form component. It makes sense for the same rule to apply here.

This will help greatly with optimizing large form sections (represented with field arrays) which are unrelated to each other.

@erikras erikras merged commit 638d825 into final-form:master Aug 4, 2019
@erikras
Copy link
Member

erikras commented Aug 15, 2019

Published in v3.1.1.

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.

FieldArray re-renders even if the subscription prop is set, the subscription prop is ignored
4 participants