jaredpalmer / formik

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

Recommended way to implement undo/redo? #2339

Open davidroeca opened 4 years ago

davidroeca commented 4 years ago

❓Question

I’m intending on migrating a form previously implemented with redux-form and redux-undo. I currently have a proof of concept implementation of undo/redo using a separate context with useReducer that relies on useFormikContext and operates similarly to redux-undo by storing intermediate values in an object like:

interface History<Values> {
  past: Array<Values>
  present: Values
  future: Array<Values>
}

Undo/redo actions simply read from past/future, setValues(), then update the history.

Is this the only way to achieve undo/redo with formik? Are there other recommended ways? One downside to this approach is that every change (I.e. every keystroke in an input) will be a new part of the history and it’s hard to group them together other than by implementing some minimum time interval between items in the history.

malyzeli commented 4 years ago

@davidroeca I'm also in need of undo/redo functionality and I'm going to start working on PoC for our project soon. I would be glad to cooperate if you are interested.

My idea for implementation was basically the same as yours, since previously I also used redux-undo. One way to solve "grouping" changes in the history might be for example using blur handlers to store only single history state per individual field change. Need to think more about individual use cases though.

Maybe @jaredpalmer will give us some ideas as well?

johnrom commented 4 years ago

That's a pretty heavy history, I would recommend instead, if it was possible in the future to subscribe to Formik's reducer itself, to build a history of changes instead of maintaining the full value list. There should also be a limit of how long prev and next can get.

Not sure if it can be implemented with the current API, since I believe we can only subscribe using onChanges, and not changes using setFieldValue(). If you aren't using setFieldValue to do anything like setting dependent values, this might be possible for you.

type FormikSetFieldValueMessage<Values> = FormikMessage<Values> & { type: 'SET_FIELD_VALUE' };
type FormikSetValuesMessage<Values> = FormikMessage<Values> & { type: 'SET_VALUES' };

// each prev and next is a "set" of changes, 
// or the full values object
export interface FormikHistory<TValues> {
    prev: Array<
        | FormikSetFieldValueMessage<TValues>[] 
        | FormikSetValuesMessage<TValues>
    >;
    snapshot: TValues,
    next: Array<
        | FormikSetFieldValueMessage<TValues>[] 
        | FormikSetValuesMessage<TValues>
    >;
}
malyzeli commented 4 years ago

@johnrom I don't see how storing FormikMessages instead of actual values might result in "lighter" history, it seems that would make history even more "heavier", or am I missing the point?

Considering storing the following data:

const stateSnapshot = { 
  field: "username", 
  value: "johndoe" 
};

// vs

const formikMessageSnapshot = { 
  type: "SET_FIELD_VALUE", 
  payload: { 
    field: "username",
    value: "johndoe"
  }
};

Edit: Ahh, okay, now I see @davidroeca was showing a snipped storing whole form data via Values - that's not what I had in mind, I believe storing just changed values should be sufficient. Sorry for the confusion!

johnrom commented 4 years ago

With my example above (which I just fixed now that I have Formik's types in front of me), here's a comparison of changes in form state with 5 fields. Note that with more fields, the heavy version will grow at a rate of changeCount * fieldCount, whereas the light version will grow only with changeCount.

// typed my name, a space, then hit undo
const heavyHistory = {
    prev: [
        { firstName: '', lastName: '', age: 0, favoriteColor: '', favoriteDinosaur: '' },
        { firstName: 'j', lastName: '', age: 0, favoriteColor: '', favoriteDinosaur: '' },
        { firstName: 'jo', lastName: '', age: 0, favoriteColor: '', favoriteDinosaur: '' },
        { firstName: 'joh', lastName: '', age: 0, favoriteColor: '', favoriteDinosaur: '' },
    ],
    currentState: 
        { firstName: 'john', lastName: '', age: 0, favoriteColor: '', favoriteDinosaur: '' },
    next: [
        { firstName: 'john ', lastName: '', age: 0, favoriteColor: '', favoriteDinosaur: '' },
    ]
}
const lightHistory = {
    prev: [
        { fieldName: 'firstName', value: '' },
        { fieldName: 'firstName', value: 'j' },
        { fieldName: 'firstName', value: 'jo' },
        { fieldName: 'firstName', value: 'joh' },
    ],
    currentState: 
        { firstName: 'john', lastName: '', age: 0, favoriteColor: '', favoriteDinosaur: '' },
    next: [
        { fieldName: 'firstName', value: 'john ' },
    ]
}

In order to make this work, we'd need the ability to pass a custom reducer into Formik, or manually override the setValues and setFieldValue fns via useFormik and a custom FormikProvider context.

malyzeli commented 4 years ago

@johnrom Yep, totally agree with that - I just didn't noticed OP was talking about storing all values.

What is the reasoning for storing full form data in currentState, considering I can read the current state from Formik context anytime (and store only the changes in my history stack)?

davidroeca commented 4 years ago

Unfortunately my form is not so simple and I'd need to track changes made by the user, or those made by automatic updates to dependent fields (the dropdowns in my form have options taken from a third party API that depend on one another). So my only option as it stands is to store all values unless formik exposes some of its internals.

davidroeca commented 4 years ago

This can be limited by what I previously mentioned (set a minimum time interval e.g. 1-2 seconds that limits updates to the history) and by limiting the length of past/future, so that the history doesn't balloon.

johnrom commented 4 years ago

I was just using OP's example where he was managing state higher in the hierarchy, which is not ideal due to constant re-rendering of Formik. Instead I propose allowing a dev to override the reducer itself with a redux-like version that supports subscription so these two can coexist at the same level and not depend on keeping context in sync.

johnrom commented 4 years ago

Being able to override and subscribe to the reducer would solve all sorts of issues like #1218 #172

Related issue: #1854

malyzeli commented 4 years ago

Is anything wrong with approach of this kind?

function useFormikHistory(props) {
  const formik = useFormik(props);

  const history = useState();
  // or useReducer, whatever

  function setValues(values, /*...*/) {
    // store whatever you need then pass values to formik
    formik.setValues(values, /*...*/);
  }

  function setFieldValue(/*...*/) {
    // same same, but different
  }

  return { 
    ...formik,
    setFieldValue,
    setValues,
  };
}

Seems cleaner to me than messing with Formik's internals...

Then of course you need to add some handler for undo/redo logic, which would internally use Formik's setValues/setFieldValue again.

I will share my results if I manage to create something usable.

davidroeca commented 4 years ago

@johnrom agreed, these issues seem pretty relevant. #1633 might also help.

@malyzeli That might solve it, but then you wouldn't have access to useField, useFormikContext, etc. and would have to write your own. Not that that's necessarily a bad thing, but you wouldn't really be using the full extent of what formik has to offer and would have to write your own versions of each. Maybe that's what we're stuck with here.

See here:

useFormik() is a custom React hook that will return all Formik state and helpers directly. Despite its name, it is not meant for the majority of use cases. Internally, Formik uses useFormik to create the component (which renders a React Context Provider). If you are trying to access Formik state via context, use useFormikContext. Only use this hook if you are NOT using or withFormik.

** Be aware that \, \, \, connect(), and \ will NOT work with useFormik() as they all require React Context.

malyzeli commented 4 years ago

@davidroeca not if you pass value of this useFormikHistory to FormikProvider, which is obviously the next step. Then you should be able to use all the stuff as usual - from child components point of view nothing really changes.

See how Formik component is implemented, ultimately it can be simplified pretty much to this:

export function Formik({ children, ...props }) {
  const formik = useFormik(props);
  return (
    <FormikProvider value={formik}>
      {children}
    </FormikProvider>
  );
}

Which means you can do this:

export function FormikHistory({ children, ...props }) {
  const formik = useFormikHistory(props);
  /* might put your event handlers for undo/redo here? */
  return (
    <FormikProvider value={formik}>
      {children}
    </FormikProvider>
  );
}

And you are back in business...

Of course you might replace children with component or render prop, whatever suits your needs, but if you are using useField and other hooks, then I think there is no reason for render props and children is enough.

davidroeca commented 4 years ago

:thinking: interesting... the main downside I see with this approach is that none of this is well-documented so it's unclear whether some of these components will change functionality or be removed in the future.

Regardless, it seems promising so I may give this a shot.

johnrom commented 4 years ago

yeah that's the best way to get started right now pending Jared's feedback on passing in a custom reducer. @davidroeca the best way to get it well-documented would be to open source your wrapper and document it there :)

malyzeli commented 4 years ago

@davidroeca well, change is the only constant - you might have to update your code in future, but that will probably happen regardless on chosen solution. :smile:

I prefer enhancing tools/libraries via this kind of "wrapping", it is definitely more flexible than digging into any internals, and also it's exactly what React hooks are good for!

If you put it into separate repo, feel free to invite me for collaboration, I would be glad to help.

davidroeca commented 4 years ago

I'll play around with this implementation a bit with my form and will check back in here when I feel like it's in a good place to open source, hopefully sometime this week :crossed_fingers:

davidroeca commented 4 years ago

@malyzeli I'm not sure how much progress you've made here but I've hit a bit of a snag with how useFormik is implemented.

It mostly has to do with the fact that certain internal methods rely on setFieldValue and there is a dependency hierarchy. At a shallow level, the following methods are derived from setFieldValue:

Only imperativeMethods relies on setValues.

Following on down this dependency hierarchy, even more methods/attributes rely on these. Since this all happens in one go, changing setFieldValue after the fact won't allow us to achieve the desired functionality (at least for my case, where I use a bunch of useFields). Let me know if you've found a work-around.

malyzeli commented 4 years ago

@davidroeca hmm good point, I didn't realise useField is NOT using setFieldValue from context but it's wired internally - was thinking just about setting values imperatively.

I will look into it more next week, now I have some other issues to solve first.

OoDeLally commented 4 years ago

I've just created a package for that. It uses Typescript and React Hooks (no redux, sorry). https://github.com/OoDeLally/formik-undo Feel free to use it and submit bugs and PRs.