xplato / useUndoable

↪ React hook for undo/redo functionality (with batteries included)
MIT License
168 stars 7 forks source link

When using setElements as dependency in useEffect it will cause infinite rendering as function setElements is constructed on every render of useUndoable #5

Closed bhavyashah98 closed 2 years ago

bhavyashah98 commented 2 years ago
React.useEffect(() => {
    const restoreFlow = async () => {
      try {
        if (selectedComplain) {
          setIsLoading(true);
          const protocol: Protocol = await getProtocol(
            selectedComplain._id!
          );
          setIsLoading(false);
          if (protocol) {
            const { nodes, edges, viewport } = protocol.protocol;
            setElements({
              nodes,
              edges,
            });
            setViewport(viewport);
          } 
        } else {
          setViewport({ x: 0, y: 0, zoom: 1 });
          setElements({
            nodes: [],
            edges: [],
          });
        }
      } catch (err) {
        console.log(err);
      }
    };
    restoreFlow();
  }, [setElements, selectedComplain, setViewport]);

Facing issue while fetching saved protocol.

Initially, I was using useState to manage the state of nodes and edges. Firstly I will fetch the existing protocol from the database and will set nodes and edges in useEffect of the component, this will trigger re-render and my React Flow component will render with applied changes in nodes and edges.

Now, when using setElements, it will change the state of useReducer in useUndoable and hence change setElementsfunction which in turn re-renders useEffect (as setElements is a dependency of useEffect). Hence, it will change the reference and will result into infinite rendering.

xplato commented 2 years ago

One of the complicated aspects of this issue is that the setElements function is, by design, meant to change on every render (more or less). One of the behaviors is that you can pass a callback function to setElements to which the present state is passed as a parameter.

I did overlook wrapping the setState function in useCallback (not sure why), but that wouldn't help this issue anyway, since the state.present value is used inside of it.

To be honest, I think the best way to handle something like this is to export another static function that doesn't have a "functional updater" form in the way the normal one does.

It doesn't look like you're using the functional updater; you're passing values directly. I think the best solution in your case is for me to write a staticSetState function that only accepts a value and doesn't need to change with every state update.

xplato commented 2 years ago

Since it should fix your issue, I went ahead and added a new static_setState function which should suit your use-case.

It's provided in the object that the hook exports. Usage could look like:

const [elements, _, { static_setState }] = useUndoable();

static_setState(elements.filter(...));

The reason this works is because this function doesn't depend on the present state, and thus doesn't need to be changed.

Let me know if this fixes your issue!

bhavyashah98 commented 2 years ago

Thanks for the quick solution.

I have done somewhat similar, removing setElements as a dependency from useEffect.

I am facing issues when I use setElement multiple times in the same function. As useReducer performs batch updates, not able to get the updated value of elements when using setElements in state.present. While I was getting updated previous state while using setState.

xplato commented 2 years ago

I mean, every render creates a snapshot of state in your function. The functional updater works because that would be re-written, given you the most up-to-date state there is.

With this static issue, you might be able to create a reference with useRef for the "latest" state value.

Try:

const [elements, _] = useUndoable();

const latestState = useRef(elements);

Give that a try and let me know if it works for you.

bhavyashah98 commented 2 years ago

I think the solution you have mentioned will not work as still I need to update latestState ref in setState which I can't access from my code.

Currently, I have added stateRef in useUndoable and passed that to reducer, it's working fine for me.

xplato commented 2 years ago

Ah yes, you're right, I see.

That's a good idea, passing a ref to useUndoable. I'm glad to hear this is working for you.

Well, if you come upon any other issues, please let me know! Thanks!

bhavyashah98 commented 2 years ago

I have created MR for this, which I can raise here if you like.

bhavyashah98 commented 2 years ago

Moreover,

We can add one more feature that provides support for skipping action performed to be added in past state. This will be quite useful as it will take care of the following cases:-

  1. We want to skip react-flow action when changes are in dragging state (as we only need drag start and drage stop state)
  2. If I imported the whole flow and if I want to undo it, currently it will undo nodes and edges one by one rather than undo to the previous state.
  3. Same goes for selecting nodes as well.

What we can do is we can accept the ignoreAction prop in setState, which will add action to the present state but will not make any changes to the past and present state if ignoreAction is set to true.

xplato commented 2 years ago

This is actually a wonderful idea. I'll add it now!

xplato commented 2 years ago

I have created MR for this, which I can raise here if you like.

What do you mean by this?

xplato commented 2 years ago

Alright, commit d332e759 added the ignoreAction option (to both setState and static_setState)

bhavyashah98 commented 2 years ago

I have created MR for this, which I can raise here if you like.

What do you mean by this?

I have already created a merge request which will take care of stateRef, can you give me access to create branch for the same

xplato commented 2 years ago

Well, that depends on what changes are made. You can either fork me and create a PR or send me an archive of your changes. If it breaks any existing API, I won't be able to merge it.

bhavyashah98 commented 2 years ago

Cool, created PR. You can review it otherwise changes seems fine to me.

xplato commented 2 years ago

I am facing issues when I use setElement multiple times in the same function. As useReducer performs batch updates, not able to get the updated value of elements when using setElements in state.present.

I am not sure what this means entirely.

I understand that the ref will make the state.present value constantly up-to-date, but these new changes will break the existing control-flow of certain projects.

In one of my projects, for example, I more or less depend on the sequential mutation of the state, as opposed to a constantly updated value like a ref.

I spent some time looking into the creation of a new hook and the integration of your changes, but I don't think I can accept them. Using useRef fundamentally changes the internal state representation of useUndoable.

Adding a new hook doesn't really work either because the reducer has to be the same (unless we duplicate the entire file, but I'm not doing that).

My only suggestion would be to publish your fork on NPM using something like use-undoable-ref. If you do so, I'll link to your project in the README so that people in your situation can be aware that there is a solution.

I unfortunately have to close your PR, mainly because of what I've expressed here and due to the breaking changes they make. Thanks again for your time! I hope you end up publishing this fork!