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

Allow filtering state properties before save it to history #237

Open
GeorgeGkas opened this issue Aug 19, 2019 · 3 comments
Open

Allow filtering state properties before save it to history #237

GeorgeGkas opened this issue Aug 19, 2019 · 3 comments

Comments

@GeorgeGkas
Copy link

GeorgeGkas commented Aug 19, 2019

Intro

The current version 1.0.0-beta9-9-7 does not provide a solution to customize the current state before saving it to history (thought insert). This is a crucial feature when we have a complex reducer with high update rate. In such cases, we can't break the reducer to multiple parts or filter out actions, cause the coupling between them is high. Such use cases can be found in graphic applications where the trigger of an operation dispatch multiple actions. Consider the following example:

Use case: A simple graphic app

Bellow is a simple HTML canvas

Pressing down the n key in our keyboard allow us to create nodes

We can select a node by clicking on it

We can create a temporary arrow by holding down the SHIFT key

By clicking another node, when have already selected one, we can create an edge between the two nodes

Dragging one node dispatch two actions: one to update the position of the selected node and one to redraw the arrow

Here is a simple state representation of the above use case:

const initialState = {
  // contain objects, each representing a node
  nodes: {},

   // the current selected node
  selectedNode: null,

  // The cursor position, helps when we need to show the temp arrow
  cursor: {
    x: 0,
    y: 0,
  },
  
   // if we've selected a node and hold the SHIFT key then we have to create an arrow
  drawTempArrow: false,

  // contain objects, each representing an arrow
  arrows: {}, 
}

The problem

The difficult part to manage the above state is when the properties relate to each other. Consider that the user has selected a node, this action updates the selectedNode property, and decides to UNDO. What will happen is that the selected node will be saved to the history as is. When the user dispatch REDO, the state now can be entirely different, due to reasons outside the Redux store. This might break the whole app.

Solution

The solution to the above problem is to reset the selectedNode property to null before save it to the history.

What I propose is to add one more property filterStateProps to the configuration of undoable enhancer. The property will be an function filterStateProps (currentState) with a parameter to the current unsaved state, i.e. the state before inserting it to history. The return value will be the final state that will be saved. Here is an example using this solution:

/**
 * Redux root reducer.
 */

import { combineReducers } from 'redux'

import undoable from 'redux-undoable'
import editor  from './app/editor'

export default () =>
  combineReducers({
    editor: undoable(editor, {
      limit: 10,
      filterStateProps(state) {
        return {
          arrows: state.arrows,
          nodes: state.nodes,
          selectedNode: null,
          cursor: {
            x: 0,
            y: 0,
          },
          drawTempArrow: false,
        }
      },
    }),
  })

This approach has a number of advantages:

  • Backward compatible feature
  • Easy API
  • Easy implementation without "touching many things"
  • Useful in such use cases
  • The best solution if we need to manipulate the state tree before save to history

I strongly believe that a feature like this can make the difference when working with apps that require frequent updates. I have already implemented and tested this approach to my own projects and works beautifully.

@CTam8
Copy link

CTam8 commented Dec 4, 2019

I also have a use case for this, any status on this?

@GeorgeGkas
Copy link
Author

You can include the feature using git sub-modules. Create a folder third-party in the root of the project. Then create a .gitmodules file, also in the root folder, with the following content:

[submodule "third-party/redux-undo"]
	path = third-party/redux-undo
	url = https://github.com/GeorgeGkas/redux-undo.git
	branch = filter-state-properties

In package.json update the redux-undo entry to link the submodule like so:

"redux-undo": "file:third-party/redux-undo",

Last, fetch the submodule:

git submodule update --init --recursive

Every time someone clones your repo, he needs to also fetch the submodules using:

git clone --recursive <YOUR_REPO_URL>

@nmay231
Copy link
Collaborator

nmay231 commented Jan 12, 2020

Apologies for the delayed response.

I see the use case for this addition and that you put some work into submitting a PR. However, I think I see another solution using existing features of redux-undo.

You mentioned you couldn't use filter, but maybe you could filter all actions except a "commit" action that persisted changes to history. For example, you only include the actions that create a node, draw a permanent arrow, or place a node. The actions that draw a temporary arrow, select a node, or update the position of a node will be filtered.

This could be as simple as

case 'UPDATE_NODE_POSITION':
  // Update the position. This action is filtered and not added to history
  return { ...state, nodes: updatedNodes, selectedNode: action.selectedNodeId };
case 'COMMIT_NODE_POSITION':
  state = { ...state, selectedNode: null };
  return state;
  // Now there should be no "tainted" state that will break the app on undo, redo

// ...

undoable(reducer, { filter: includeAction(['COMMIT_NODE_POSITION', '...']) });

I think something along those lines should work for your use case. If not, we can certainly discuss this addition further. Though if it does need to be added, we should change the name filterStateProps to nullifyStateProps or something so it is not confused with filter.

Also, I am working on a change that should allow users to get an easy hook into the state in a more general way. You can see the initial API I have in mind here.

Thanks for your patience 👍

@nmay231 nmay231 mentioned this issue Feb 20, 2020
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants