formik icon indicating copy to clipboard operation
formik copied to clipboard

Formik can useTransition from React 18 in order to massively increase performance and fix its most major issue

Open adam-thomas-privitar opened this issue 2 years ago • 21 comments

Feature request

Current Behavior

Typing in a field causes a synchronous rerender on everything that is receiving the value, which means in large applications typing in the fields feels slow and jerky. This has been the plague of the formik model and one big reason why react hook form etc / uncontrolled form libs became popular.

Desired Behavior

Typing (or other interactions with formik state) is always quick no matter how big your app is. We should no longer have to care about the rerenders whilst getting the nice DX of controlled form state. This is huge!

Suggested Solution

Formik would wrap its all of its developer-facing state with useTransition. It could still maintain an "accurate" version internally. With the power of React Concurrency this would mean all rerenders caused by formik would not block user input, leading to a massive easy win on big apps.

Interacting with forms will feel snappy and immediate, no matter how many rerenders or the complexity of those rerenders you got going on in your app.

Unless I'm wrong (god please, I hope I'm not wrong).

Who does this impact? Who is this for?

Anyone

Describe alternatives you've considered

Uncontrolled form libs kind of suck if youre wanting to build complex form flows as its traded a lot of other stuff in to make it happen.

Memoization sucks as has large overhead in terms of developer time and defining memo functions.

adam-thomas-privitar avatar Mar 29 '22 20:03 adam-thomas-privitar

Most major performance issues are resolved in my v3 umbrella PR. Unfortunately it was never released by the maintainer.

A separate renderer will be required for useTransition. I have a plan for this as well but it may require a full fork since this project has gone silent.

I agree that uncontrolled libs aren't ideal in all scenarios and this project is the best I have found aside from the performance issues in v2.

johnrom avatar Mar 29 '22 20:03 johnrom

@johnrom Yeh I'm familiar with your PR! Its a real shame it didn't get merged. I think youd get a lot of traction for a proper community fork of this project.

Im guessing though that useTransition, is basically the easiest possible way to eliminate the perf issues though whilst trading in (nearly?) nothing?

adam-thomas-privitar avatar Mar 29 '22 20:03 adam-thomas-privitar

(my second main issue with formik is no type safety on dot notation paths which can be solved with typescript template literals -- but i digress) https://www.typescriptlang.org/play/?ts=4.1.0-pr-40336-88

adam-thomas-privitar avatar Mar 29 '22 21:03 adam-thomas-privitar

I haven't actually found any tradeoffs in my PR or run into any major issues. I would probably recommend trying it out. UseTransition will be great in concurrent mode but I think I'd have to architect the library in such a way as to support alternate renderers. My v3 PR uses refs extensively which, if I'm not mistaken, are not compatible with useTransition.

I haven't actually switched over to concurrent mode so I'd probably need more experience there before I came up with a final solution.

johnrom avatar Mar 29 '22 21:03 johnrom

Could it be better to dump the refs entirely and go all in on concurrency? Im tempted to just give it a go on Formik 3 beta as it seems low effort.

adam-thomas-privitar avatar Mar 29 '22 21:03 adam-thomas-privitar

I also have a PR for strong types over v3.

johnrom avatar Mar 29 '22 21:03 johnrom

(I guess it would turn any listening component tree to a concurrent one -- but I think it wouldnt be a bad thing. But like you, I'm not well versed yet)

adam-thomas-privitar avatar Mar 29 '22 21:03 adam-thomas-privitar

I don't foresee concurrency being something everyone will be able to dive into in a meaningful way without the entire React community getting used to it. Kind of like adopting Async or ESM. The problem is we can't create only a concurrent version of the form library, or it will only be usable by a subset of early adopters.

If I recall, v3 isn't a problem in concurrent mode because it doesn't use refs during render, only during callbacks and effects. But I haven't been able to do super-thorough testing. useTransition might even work with it. But we can't create a useTransition-based version of the library for the above reasons -- we can't use useTransition without everyone moving to a version of React that includes useTransition and also I'm not sure it will "just work" with concurrent mode switched off. Thus, it'd probably need an alternate renderer entirely using useMutableSource and useTransition.

You can check out the last v3.x betas I will publish here:

  • 3.0.0-rc1 without Strict Types.
  • 3.1.0-types13 with Strict Types
    • requires a modern TS version to use Template literal types, will fall back to weak types with older versions
    • it is much more experimental in that some types may or may not work 100%. your mileage may vary. you can see a full list of the types I tested against in the file type-checks.tsx -- I think only one of them is broken.

If I do fork this I will be vastly simplifying how Field works to reduce the complexity of the types by 1000-fold.

johnrom avatar Mar 29 '22 21:03 johnrom

Hmm yeh. I guess what I proposed is a technically breaking change -- but React release notes state:

In our testing, we’ve upgraded thousands of components to React 18. What we’ve found is that nearly all existing components “just work” with concurrent rendering, without any changes. However, some of them may require some additional migration effort.

This, tbh is a price worth paying for many formik-users I think (new Formik 4 which is concurrent rendering only and only works on React 18 -- its the future afterall). This works for the problems I currently have, but agree it might not be for everyone.

Willing to look deeper into what it'd take to do that and perhaps raise a dumb formik-concurrent version of this lib for people wanting to the follow the road im suggesting.

adam-thomas-privitar avatar Mar 29 '22 21:03 adam-thomas-privitar

I would personally fight against the desire to gut Formik and rewrite it for concurrency. The only way I would implement something like this is if it can be implemented in a way like:

const concurrentFormLibrary = createFormLibrary()
  .withRenderer(concurrentFormLibraryRenderer);
  // or 
  // .withRenderer(refBasedFormLibraryRenderer);

In my opinion without promoting extensibility any new project is likely to end up just like this one.

johnrom avatar Mar 29 '22 21:03 johnrom

Yeh I don't necessarily disagree. I'm looking for a cheap and dirty solution to my problems short-term, and suspect I'm not alone. Any fork I'd produce would exist only to serve that purpose and not necessarily be the panacea.

Totally, what you propose is more flexible and community-ready. This project needs willing volunteers to create a "proper" fork, but I think that would come after the initial burst of work naturally.

adam-thomas-privitar avatar Mar 29 '22 21:03 adam-thomas-privitar

Try out one of my PRs and see if they solve your problems first. They were not cheap and dirty, but so far as I can tell they are completely backwards compatible. Feel free to comment on the PR if you find any issues. If you're using a codebase with Formik already migrating should be painless (assuming you aren't trying to tighten up your Types. it's amazing how the strongly typed PR will reveal the issues in a codebase).

johnrom avatar Mar 29 '22 21:03 johnrom

Cool! Yeh I'll check it out. I'd probably lean on the non-types one first before inevitably attempting the typed version and realising how many issues we have 😛.

At the same time, I might dig in to "formik hacked to shit with concurrent" if only for my own learning opportunity and share back any findings.

adam-thomas-privitar avatar Mar 29 '22 21:03 adam-thomas-privitar

can you release something please instead of us delaying all our work and wasting our time because someone didn't want to merge a pull request in a repo that has 30k stars, and we only went with that repo because "poor us" thought that okay this is a popular lib in the JavaScript world IT MUST BE frequently updated ... but no, take vim and neoVim as inspiration, you have my support if you have better solutions. its not just about performance, the latest release of formik is 1 FULL major react version behind .. and the types are 2 majors behind, and its creating problems and conflicts and warnings and memory leaks left and right ...

In the end its just JavaScript form validation and its not a big deal to migrate away, but my disappointed stems from the idea that if you decided to take a path in publishing and open sourcing code and received a relatively major support from the community, you are obligated to continue serving, or delegate the tasks at least, no one is forcing you, its just whenever you get a right to do something it should be followed by a duty towards something else.

I hope that makes sense, appreciate all your efforts, wishing everyone the best and if there were a story I missed that can respond to my comment I apologies in advanced, if there is not, take it from me as a friend not a critique.

upq avatar Apr 19 '22 22:04 upq

Please don't tag me to rant, I can offer you the option to use my PR at your own risk, but I'm not the repository owner and will not provide support unless I create an official release.

You're free to add any issues you have with the PR itself to the PR and I'll try to help. Thanks for understanding!

johnrom avatar Apr 19 '22 22:04 johnrom

I just wanted to update on my findings. I spent some time doing a horrendous hack that essentially did something like this:

  • Wrapped all of the return values of useFormik with a useTransition
  • Apart from a new "currentValues" state i hacked in, which is not wrapped in transition. useField was changed to use this one. This ought to mean render that updates field value would be prioritised.

Result: explosion -- call depth exceeded. Though I've got a feeling this is something in the application I was testing my hacks against. I need to look more. I did however workaround by wrapping only the read-states and not the closures with transition. "Yes!" I thought. However, my test suite on the app (which types a lot in the fields) ran about a third slower.

I feel I need to keep digging as the results are so unexpected. Not a very scientific approach that I took -- I need to spend some more time with simple test cases. I was over-ambitious in attempting to try it against my massive application.

Also, on the above, johnrom has not released his PR branch as a release unto itself, he has 0 obligation to support it. If he did do a "release" I'd fully support but I dont think using language like "obligation" is an encouraging factor and is part of the reason why people dont want to do open source. People are working for free here. Sure its frustrating this project has died, but those are calculated risks we take in open source which may have not paid out. It happens and will happen again.

adam-thomas-privitar avatar Apr 20 '22 14:04 adam-thomas-privitar

@adam-thomas-privitar I'm glad someone tried it and we can stop with the "omg lol like, usetransition pls so ez"

Everything that is dispatched in order to cause a render would need to useTransition but Formik's internal state itself should be independent of React.

Thus the internals of React should kind of look like my PR:

  const [isPending, startTransition] = useTransition();

  /**
   * Each call to dispatch _immediately_ updates the ref.
   * It also batches updates to all subscribers.
   */
  const dispatch = React.useCallback(
    (msg: FormikMessage<Values>) => {
      stateRef.current = formikReducer(stateRef.current, msg);
      // calculate computed state once on each dispatch,
      // so it can be reused across subscriptions
      computedStateRef.current = getComputedState();

      startTransition(() => {
        subscriptionsRef.current.forEach(callback => callback());
      })
    },
    [stateRef]
  );

In my PR, it's possible using startTransition would be as easy as using a different batch command:

Like this file for the React Native implementation:

https://github.com/johnrom/formik/blob/johnrom/v3/packages/formik/src/index.native.tsx

Except replace:

// Borrowed from React-Redux, License MIT.
// https://github.com/reduxjs/react-redux/blob/d4e4eba9ccbd488b103b3c5625a37e15b1427d11/src/utils/reactBatchedUpdates.native.ts
// https://github.com/reduxjs/react-redux/blob/d4e4eba9ccbd488b103b3c5625a37e15b1427d11/LICENSE.md
//
// Use this file by setting an alias in webpack or something: `alias: { 'formik': 'formik/index.concurrent' }`
//
export * from './exports';
- import { unstable_batchedUpdates } from 'react-native';
+ import { startTransition } from 'react';
import { setBatch } from './helpers/batch-helpers';

// I saw some documentation that you can call startTransition without useTransition / pending state.
// Not sure if this is true.
// This might actually block interactions with input elements 
// without manually adding `event.target.value = newValue` to handleChange
- setBatch(unstable_batchedUpdates);
+ setBatch(startTransition);

Unfortunately, if you do this, you actually drop out of controlled inputs until the transition is complete.

johnrom avatar Apr 20 '22 16:04 johnrom

I think in a library of controlled inputs like formik, the only real use case for useTransition is for effects after the onChange, because like mentioned above the very concept of a controlled input is blocked behind a transition. Still, validation and other effects are probably the most expensive part of Formik so it's still a very good use case. Maybe adding a Priority to Dispatch which could selectively startTransition or not startTransition would be the best way to go.

onChange = () => dispatch("update-value", HIGH), dispatch('request-validation', LOW).

johnrom avatar Apr 20 '22 16:04 johnrom

Tags removed, i didn't tag you to rant lol i tagged you to see it ... good luck

upq avatar Apr 22 '22 02:04 upq

I think in a library of controlled inputs like formik, the only real use case for useTransition is for effects after the onChange, because like mentioned above the very concept of a controlled input is blocked behind a transition. Still, validation and other effects are probably the most expensive part of Formik so it's still a very good use case. Maybe adding a Priority to Dispatch which could selectively startTransition or not startTransition would be the best way to go.

onChange = () => dispatch("update-value", HIGH), dispatch('request-validation', LOW).

Yeh, thats the angle I took with my hack (in regards to your first sentence). All state return from Formik was wrapped in useDeferredValue (sorry made a mistake earlier, I didn't use useTransition, I just did it the poor mans way to see if it would work) meaning anything hooked into it was lower priority. Apart from the field rendering useField cos I created a non-defferred copy of the values to wire that in. I know...disgusting.

I still need to come back to this. Im not so much looking at using that as a real solution -- more that I want practical relatively-low-effort proof deferring renders of anything that listens to formik state (apart from the fields) would fix my laggy-input problem. It sounds like it should but I want to sanity check it. I think my app is probably a decent barometer of the typical app that causes this issue so if it was to work, more than likely it'd work for lots of folks. Once I had that proof, I could perhaps look at doing it properly.

Definitely, youd probably want more granular control of what state updates are high/low priority rather than baking in an assumption like "any state updates related to forms that are not the form input itself are deferred". I agree with pretty much all your API proposals thus far.

On the topic of your PR/potential community fork -- I'd definitely love to contribute to such a thing. What are your thoughts regarding such a fork (assuming youre considering it -- feel free to shut me down if your not!!)? What are some of the blockers -- maybe I could help with those.

BTW I still need to try your PR in my app -- I need to roll back the coupling to the old beta hooks first.

adam-thomas-privitar avatar Apr 23 '22 12:04 adam-thomas-privitar

The main problem honestly is that I only spend about 4 hours a week on non-work projects, and right now my focus is on a separate project. I have also been experimenting with documentation and how to effectively maintain documentation for projects since I have my hand in a few of them, which resulted in Nmbl Code Snippets.

Even coming up with a plan for a fork is a lot, especially re: licensing concerns as well as branding concerns (obviously it cannot be branded as Formik since I don't own any rights to the name).

johnrom avatar Apr 23 '22 20:04 johnrom