jaredpalmer / formik

Build forms in React, without the tears 😭
https://formik.org
Apache License 2.0
33.98k stars 2.79k forks source link

FastField is rendering on every value change #1759

Open thupi opened 5 years ago

thupi commented 5 years ago

🐛 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 commented 5 years ago

I created a similar issue here https://github.com/jaredpalmer/formik/issues/1739 and someone gave a temporary solution if you want it now.

thupi commented 5 years ago

@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 commented 5 years ago

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.

wpq0 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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).

https://github.com/reactjs/rfcs/pull/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.