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

Register only once for useField hook #727

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ryank109
Copy link

Hi, I'm trying to address an issue describe here on issue #667 to improve performance on initial registration of the field.

Also there's a final-form PR that goes with this change: final-form/final-form#319

Again, to describe the findings, with useField hook, it was using registerField to register for the first time to get the initial values synchronously, then unregister to throw away the initial subscription to get the initial state, then register again for subscribing to the subsequent updates to the form. And each time it registered, it fired off notify subscription on all previously registered fields and for the current subscription. So, if you are trying to render 100 fields on the screen, that's 2 registration per field and 10100 or n(n + 1) notifications.

So I wanted to separate the registration and subscription to make sure that registration and initial state is calculated during that registration, and subscribe in a separate call to the field states. With this change, I've reduced the number of notifications to 0, and only registering the field once. Another side effect of this change was the fix for this issue as well: #639. The change I made does not unregister the field when it's unmounted.

But..., since I wanted to make sure that it works with currently supported behaviors, I did introduced keepFieldStateOnUnmount property to useField/Field to make sure that the current logic of removing/unregistering the field on unmount still works.

@ryank109 ryank109 requested a review from erikras February 3, 2020 14:37
@erikras
Copy link
Member

erikras commented Mar 31, 2020

Can you see if this is still a problem after v6.4.0?

@ryank109
Copy link
Author

ryank109 commented Apr 4, 2020

@erikras
Yes, seems like it's still a problem. I've created a repo to test the performance of initial registering: https://github.com/ryank109/react-final-form-pref

react-final-form@6.4.0 with 3000 fields ~7-8s
with this fix, with 3000 fields ~1s

[Edit - 4/6/2020]
To elaborate more, I think silent mode that you added on the first setState initialization did improve the performance a little from the previous version, because you narrow down the subscription call on the first register call. However, the useEffect call that comes right after the setState is registering and still calling to notify every subscription registered so far. Which is still a large operation if there's many, many fields registered on the screen.

Could you explain the reason behind why notifications on register matters? Maybe I'm not understanding the case that you were trying to cover. Still, I was make all the unit test pass without calling to notify subscriptions.

Thanks.

P.S. I'll update this PR after your feedback, because I'm not sure with my changes silent mode is necessary or not. Thanks!

@ryank109
Copy link
Author

@erikras Any recommendations?

@ra2dev
Copy link

ra2dev commented Apr 30, 2022

@erikras
Do you have any objections against this functionality? I'm asking since i want to finish work with this PR and if possible merge it.

@Dragomir-Ivanov
Copy link
Contributor

@ra2dev What is the status of this PR. It will be really useful to me, because I have a form, separated in different tabs?
My current workaround if anyone curious, is to keep registered all Form fields, all the time rendering null.

@erikras Since FF is not actively maintained, can to transfer rights to anyone who wants.

@ra2dev
Copy link

ra2dev commented Sep 15, 2022

@Dragomir-Ivanov
Was not able to dedicate time on this PR, resolved issue on my side by creating separate context to track registered fields that are located on "hidden" tabs (temp solution).
Next month planning to work on blog post related to performance of form libraries, hopefully will be able to play with it and mb provide solution

@Dragomir-Ivanov
Copy link
Contributor

Thank you @ra2dev , and sorry for the late response.
The issue with "hidden" fields, is that being hidden they don't loose their value, but loose their state, i.e.: dirty, touched, modified, etc.. This makes working with FF hacky. It seems that one can't just hold a reference to a FormApi, without any form rendering, and persist, form state.
I am already thinking in favor of React Form Hook, and will try to swap on first occasion.
Please include RFH in your blog as well.

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.

4 participants