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

FastField is rendering on every value change #1759

Open
thupi opened this issue Aug 18, 2019 · 7 comments
Open

FastField is rendering on every value change #1759

thupi opened this issue Aug 18, 2019 · 7 comments
Labels
Focus: Performance Enhancing Field and Form performance / rendering stale

Comments

@thupi
Copy link

thupi commented Aug 18, 2019

🐛 Bug report

Current Behavior

The FastField component renders even though its another fields value that is being changed.

Expected behavior

FastField components should only rerender when its own value is changed.

Reproducible example

The very wired part of this bug is that this works as expected on codesandbox. However, when i make a new clean project using CRA it doesn't :-/

Codesandbox: https://codesandbox.io/s/formik-2-rwlhv
Clean CRA project: https://github.com/thupi/formik-fast-field-sample

Additional context

Look for the console.log messages in both of these samples i provided above. The CRA sample console.logs both fields on every change while codesandbox works as expected.

I Thought this was due to typescript so i tried with at clean js project too but made no difference.

Software Version(s)
Formik 2.0.0.rc-12
React 16.9.0
TypeScript 3.5.2
Browser Chrome
npm/Yarn Yarn 1.15.2
Operating System MacOS Mojave
@tobeycodes
Copy link

tobeycodes commented Aug 18, 2019

I created a similar issue here #1739 and someone gave a temporary solution if you want it now.

@thupi
Copy link
Author

thupi commented Aug 27, 2019

@schrapel I see, however in my case it is quiet consistant with only rendering all fields. it never rendered 6 times in my case :-) But indeed very similar :-)

@johnrom
Copy link
Collaborator

johnrom commented Aug 31, 2019

I'll add some context here for whever wants to tackle this. Basically the only difference between FastField and a regular Field (if I understand it correctly, code is not in front of me!) is that when FastField itself changes, it keeps its changes locally and only propagates them to Formik when the field is blurred. This means changes to a FastField do not have a performance detriment to the Form at large. However, FastField must react to any changes that come from Formik's state, or its own state will be stale. Therefore, if you have a separate, plain Field which updates Formik on every change, FastField will have to re-render each time. This is because FastField, like Field, gets a lot of objects from Formik's state, and checking if they are actually different will actually slow down faster forms -- it's a trade-off. If implemented, these checks will need to be perfect in order to prevent fields from getting stale.

This, of course, isn't ideal, and is a great low hanging fruit where we can improve.

@schrapel your issue is slightly different, and I'll comment on your issue.

@johnrom johnrom added the Focus: Performance Enhancing Field and Form performance / rendering label Aug 31, 2019
@wpq0
Copy link

wpq0 commented Sep 12, 2019

Seeing that FastField is now just an alias for Field
. Do we need a different way to achieve the same effect, like useField and memo?

@johnrom
Copy link
Collaborator

johnrom commented Sep 13, 2019

@wpq0 if I were to guess, FastField will be reimplemented in v2. for now you'd probably have to make your own component like <Field as={MyFastFieldImplementation} /> where MyFastFieldImplementation doesn't trigger onChange until it's blurred. It's definitely not ideal, but v2 hasn't been released yet.

example PR: #1772

@mvestergaard
Copy link
Contributor

mvestergaard commented Oct 30, 2019

v2 has now been released, and this is still a problem. I actually think there are way worse performance issues in v2 than this thread originally seems to indicate.
I can demonstrate it in these two examples:

v2.0.3: https://codesandbox.io/s/vigorous-wood-esy70
v1.5.8: https://codesandbox.io/s/keen-bouman-sx5r0

In v2, any change, to any field (FastField or Field) results in two renders of all fields in the form.

@johnrom
Copy link
Collaborator

johnrom commented Oct 31, 2019

In my opinion (I don't speak for the maintainers), the issue here is that FastField is a performance optimization (read: hack) that breaks the way fields are assumed to work. The ideal solution would be to optimize fields in order to select a piece of the context which they require. Lots of packages are having this issue with React v16 right now, and the React team seems like they're not interested in fixing it (or it's not a high priority).

reactjs/rfcs#119

I think Formik could have a FastField wrapper which does this type of memoization internally, but a lot of the performance problems aren't coming from the "render" actions, but userland code that occurs during render which can be memoized. It's a question of whether implementing a fix like this would provide any benefit over userland memoization and if the React team will address this with a better API and make all that effort for nothing. If instead of a simple render count (renders are cheap), you can provide a scenario which cannot be optimized in userland and is providing a performance issue, I'd gladly take a look. In the case of your sandbox above, the existence of RenderCountingInput destroys the performance optimizations within React itself, since renders cannot be optimized due to changing text in useEffect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Focus: Performance Enhancing Field and Form performance / rendering stale
Projects
None yet
Development

No branches or pull requests

5 participants