Skip to content

Conversation

@ndbroadbent
Copy link

See: #95, and #95 (comment)

The past and present arrays include full copies of the state whenever a user makes a change. It's not a problem in the browser because they share the same memory references, but it's a huge problem when you need to serialize that data. This library is already specific to redux, and redux-undo is the most common way of adding undo/redo functionality to a React/Redux application. So maybe this makes sense as a good default. Otherwise most redux-undo users will end up with 413 responses, or no state at all (when using the code in this PR (#95).)

@codecov-io
Copy link

codecov-io commented Oct 14, 2018

Codecov Report

Merging #96 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #96   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          26     45   +19     
  Branches       10     21   +11     
=====================================
+ Hits           26     45   +19
Impacted Files Coverage Δ
index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ab4e52...4c7c841. Read the comment docs.

@captbaritone
Copy link
Owner

Cool! I don’t think this deserves to be in the core library, but I appreciate that you took the time to post it so others could find it.

Maybe we could make the example directory into an examples directory with a few sub directories. Then you could include this as an example project. What do you think?

@captbaritone
Copy link
Owner

I'll close this for now. Feel free to reply if you think it should be reopened.

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

Successfully merging this pull request may close these issues.

3 participants